search

Infineon / infineon-design-system-stencil

https://infineon.github.io/infineon-design-system-stencil/
MIT License
21 stars 8 forks source link

Enhancement: form integration for radio button checkbox and text field #1284

Open pfafffabian-ifx opened 3 weeks ago

pfafffabian-ifx commented 3 weeks ago

By creating this pull request you agree to the terms in CONTRIBUTING.md. https://github.com/Infineon/.github/blob/master/CONTRIBUTING.md --- DO NOT DELETE ANYTHING ABOVE THIS LINE ---

CONTRIBUTING.md also tells you what to expect in the PR process.

Description Adds form integration to checkbox, radio-button and text-field. Uses stencil formAssociated to implement the integration

Related Issue

1158

📦 Published PR as canary version: 23.0.1--canary.1284.b868ff709931a5f5c75807639ac089ae54d5e8aa.0
:sparkles: Test out this PR locally via: ```bash npm install @infineon/infineon-design-system-stencil@23.0.1--canary.1284.b868ff709931a5f5c75807639ac089ae54d5e8aa.0 # or yarn add @infineon/infineon-design-system-stencil@23.0.1--canary.1284.b868ff709931a5f5c75807639ac089ae54d5e8aa.0 ```
github-actions[bot] commented 3 weeks ago

PR Preview Action v1.4.7 :---: :rocket: Deployed preview to https://Infineon.github.io/infineon-design-system-stencil/pr-preview-react-example/pr-1284/ on branch gh-pages at 2024-07-01 13:34 UTC

github-actions[bot] commented 3 weeks ago

--STORYBOOK-PREVIEW--

github-actions[bot] commented 3 weeks ago

--EXAMPLE-APPS-PREVIEW-- PR Preview URLs:

tishoyanchev commented 2 weeks ago

@pfafffabian-ifx The values are still not submitted. I don't understand the changes that you have made, and their intended goal. You have removed the name property, and included the attachedInternals decorator. It is unclear to me what does this decorator do, and what is it supposed to achieve? Could you please explain your changes.

pfafffabian-ifx commented 2 weeks ago

@tishoyanchev I follwed the stencil documentation for form-associated components. The name property doesn't need to be explicitly mentioned, however I can add it again if prefered.

I tested whether it works with the E2E tests and with this form in the index.html:

    <form id="testForm" action="http://www.example.com" method="get" >
      <ifx-text-field id="formName" name="name"></ifx-text-field>
      <ifx-radio-button id="formRadio" name=radio></ifx-radio-button>
      <ifx-checkbox id="formCheckbox" name="checkbox"></ifx-checkbox>
      <ifx-button type="submit" theme="default" size="s" disabled="false" icon="false">
        Submit form
      </ifx-button>
      <ifx-button type="reset" variant="secondary" theme="default" size="s" disabled="false" icon="false">
        Reset form
      </ifx-button>
    </form>
tishoyanchev commented 2 weeks ago

@tishoyanchev I follwed the stencil documentation for form-associated components. The name property doesn't need to be explicitly mentioned, however I can add it again if prefered.

I tested whether it works with the E2E tests and with this form in the index.html:

    <form id="testForm" action="http://www.example.com" method="get" >
      <ifx-text-field id="formName" name="name"></ifx-text-field>
      <ifx-radio-button id="formRadio" name=radio></ifx-radio-button>
      <ifx-checkbox id="formCheckbox" name="checkbox"></ifx-checkbox>
      <ifx-button type="submit" theme="default" size="s" disabled="false" icon="false">
        Submit form
      </ifx-button>
      <ifx-button type="reset" variant="secondary" theme="default" size="s" disabled="false" icon="false">
        Reset form
      </ifx-button>
    </form>

For some reason it worked this time. I will review it again and then merge it. Good job.

tishoyanchev commented 2 weeks ago

@pfafffabian-ifx

pfafffabian-ifx commented 2 days ago

@tishoyanchev The element is disabled once the disabled property is present, exactly as in default HTML components, e.g. <input disabled></input>. Stencil also has an Issue about this topic and if I understood correctly, the behaviour we currently have would comply with the standard.

tishoyanchev commented 2 days ago

@pfafffabian-ifx

pfafffabian-ifx commented 1 day ago

@tishoyanchev I resolved the merge conflict and added the description for the text field. However, I somehow already saw the name property in the storybook without this change. Maybe we should consider adding something like a devcontainer, if we keep having these issues, or is that only with me?

tishoyanchev commented 1 day ago

@tishoyanchev I resolved the merge conflict and added the description for the text field. However, I somehow already saw the name property in the storybook without this change. Maybe we should consider adding something like a devcontainer, if we keep having these issues, or is that only with me?

Looks good now. I can see the prop name in Storybook now. Can you please also resolve the failed check?