Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ebay-textbox: typescript type check breaks for step and other input attributes #2356

Closed
mikehobi opened this issue Dec 27, 2024 · 3 comments
Closed

Comments

@mikehobi
Copy link
Contributor

Bug Report

eBayUI Version: 14.x

Description

Since ebay-textbox only extends "textarea", attributes such as step will fail type check

Workaround

Adding spread props w/ any

<ebay-textbox
    type="number"
    ...({ step: "0.01" } as any)
/>

Screenshots

image
@JoshuaWink
Copy link
Contributor

Bug Triage: Missing Support for step Attribute in ebay-textbox

Summary

The ebay-textbox component, which supports type="number", does not allow the step attribute. Developers using ebay-textbox with type="number" are currently encountering TypeScript errors when trying to set step.

Impact

  1. Developer Workaround: To bypass TypeScript errors, developers must use a workaround (as any), which is not ideal for maintainability or readability.
  2. Feature Incompleteness: Without step, type="number" functionality is limited, impacting user experience for numeric inputs (e.g., intervals for price, quantity, percentages, etc.).
  3. Inconsistency: The behavior of ebay-textbox deviates from standard HTML <input> expectations.

Proposed Fix

To resolve this issue, the following changes are suggested:

  1. Update Marko Tag Configuration:

    • File: /src/components/ebay-textbox/marko-tag.json
    • Add the step attribute to the component’s configuration.
    {
      "attributes": {
        "step": "string",
        ...
      }
    }
  2. Update TypeScript Props Interface:

    • File: /src/components/ebay-textbox/component-browser.ts
    • Extend the TextboxInput interface to include step as an optional property:
    interface TextboxInput extends Omit<Marko.Input<"textarea">, `on${string}`> {
        step?: string;
        ...
    }
  3. Add Unit Tests:

    • File: /src/components/ebay-textbox/test/test.server.js
    • Add a test to verify the step attribute renders correctly:
    it("renders a textbox with step attribute", async () => {
        await htmlSnap(Isolated, { type: "number", step: "0.01" });
    });
    • File: /src/components/ebay-textbox/test/test.browser.js
    • Validate that the step attribute is functional:
    describe("given a textbox with step attribute", () => {
        beforeEach(async () => {
            component = await render(Isolated, { type: "number", step: "0.01" });
        });
    
        it("should include step in the rendered input", () => {
            const input = component.getByRole("textbox");
            expect(input).toHaveAttribute("step", "0.01");
        });
    });
  4. Update Storybook Examples:

    • File: /src/components/ebay-textbox/textbox.stories.ts
    • Add an example to demonstrate the step attribute:
    export const WithStep = Template.bind({});
    WithStep.args = { type: "number", step: "0.01" };
    WithStep.parameters = {
        docs: {
            source: {
                code: tagToString("ebay-textbox", WithStep.args),
            },
        },
    };

Priority

This bug should be prioritized as High for the following reasons:

  1. It introduces TypeScript errors and necessitates workarounds.
  2. The step attribute is critical for numeric inputs, limiting functionality in real-world scenarios like pricing and quantity inputs.
  3. Fixing this aligns the component with standard HTML behavior, enhancing usability and consistency.

@JoshuaWink
Copy link
Contributor

Update:

I initially marked this as a non-blocking bug, assuming it would be a simple fix. However, after diving into the code and attempting to apply the solution, I realized this is more complex than it first appeared.

Here are the key findings:

  1. No Documentation:

    • There’s no mention of the step attribute or similar HTML attributes and their styling in the playbook.
  2. Feature Scope:

    • Supporting the step attribute involves more than fixing an oversight. It requires:
      • Updating the component’s implementation.
      • Creating new documentation and Storybook examples.
      • Adding tests and ensuring design consistency across the system.
  3. Needs Discussion:

    • This doesn’t align with a quick bug fix—it feels more like a feature addition. I’ve removed the severity and added a blocking tag as the scope needs clearer definition and alignment with the project’s standards.

Recommendation:
Let’s discuss how step and similar attributes should be integrated into ebay-textbox.

@LuLaValva
Copy link
Member

@WinkeeFace thanks for looking into this. Since step is a built-in HTML attribute and ebay-textbox is just a wrapper around the native <input> there isn't really any documentation necessary and also there aren't any style changes. We'll discuss offline and get a PR up today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

4 participants