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

Bugfix: Align Radio-Button, Checkbox and Switch to web standards #1485

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

pfafffabian-ifx
Copy link
Collaborator

@pfafffabian-ifx pfafffabian-ifx commented Sep 19, 2024

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

  • Rename the value property of Checkbox, Switch, Radio-Button to checked
  • Implement Radio Button group behaviour

Related Issue
Closes #1479
Fixes #1476 for the Radio Button

📦 Published PR as canary version: 26.0.0--canary.1485.2bf616acdcc9d0d562598edde850bf390cf42094.0

✨ Test out this PR locally via:

npm install @infineon/infineon-design-system-stencil@26.0.0--canary.1485.2bf616acdcc9d0d562598edde850bf390cf42094.0
# or 
yarn add @infineon/infineon-design-system-stencil@26.0.0--canary.1485.2bf616acdcc9d0d562598edde850bf390cf42094.0

@pfafffabian-ifx pfafffabian-ifx added bug Something isn't working major major version bump labels Sep 19, 2024
@pfafffabian-ifx pfafffabian-ifx self-assigned this Sep 19, 2024
@pfafffabian-ifx pfafffabian-ifx linked an issue Sep 19, 2024 that may be closed by this pull request
@pfafffabian-ifx pfafffabian-ifx force-pushed the 1479-radio-button-integrate-input-element branch from 42abf6d to ce93639 Compare September 19, 2024 12:18
Copy link
Contributor

github-actions bot commented Sep 19, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://Infineon.github.io/infineon-design-system-stencil/pr-preview-angular-example/pr-1485/
on branch gh-pages at 2024-10-17 09:12 UTC

Copy link
Contributor

--STORYBOOK-PREVIEW--

@pfafffabian-ifx pfafffabian-ifx force-pushed the 1479-radio-button-integrate-input-element branch from ce93639 to c532f13 Compare September 19, 2024 12:22
@tishoyanchev
Copy link
Contributor

tishoyanchev commented Oct 2, 2024

@pfafffabian-ifx

  • when radio-button is disabled, event is still emitted on click.
  • Radio-button warning message: "inputElement" State changed during rendering. This can potentially lead to infinite-loops and other bugs. In short, the inputElement State should not be mutated during rendering time.
  • remove disabled attribute from the storybook snippet of Switch and Checkbox.
  • value prop is missing from Radio-button, Checkbox and Switch. The value of the value prop is submitted in a form.
  • Remove name="search_terms" from single-select. Name prop can be set by the users.
  • Selected options are not submitted for the multi-select. Should they?

@pfafffabian-ifx pfafffabian-ifx force-pushed the 1479-radio-button-integrate-input-element branch from f23d96f to b6d348b Compare October 17, 2024 08:38
@pfafffabian-ifx pfafffabian-ifx force-pushed the 1479-radio-button-integrate-input-element branch from b6d348b to 998bcfe Compare October 17, 2024 09:03
@verena-ifx
Copy link
Contributor

The checkbox doesnt have the "disabled" arg in the story anymore. Not sure why this got removed, but the checkbox should be "disable-able"

@verena-ifx
Copy link
Contributor

@pfafffabian-ifx

  • when radio-button is disabled, event is still emitted on click.
  • Radio-button warning message: "inputElement" State changed during rendering. This can potentially lead to infinite-loops and other bugs. In short, the inputElement State should not be mutated during rendering time.
  • remove disabled attribute from the storybook snippet of Switch and Checkbox.
  • value prop is missing from Radio-button, Checkbox and Switch. The value of the value prop is submitted in a form.
  • Remove name="search_terms" from single-select. Name prop can be set by the users.
  • Selected options are not submitted for the multi-select. Should they?

@pfafffabian-ifx you renamed value to checked, as I could see. Fine with me, if this is in accordance with the web standard. But yes, @tishoyanchev is right, value is still used in the code as well. What do you mean by the select options not being submitted?

@tishoyanchev
Copy link
Contributor

The checkbox doesnt have the "disabled" arg in the story anymore. Not sure why this got removed, but the checkbox should be "disable-able"

@verena-ifx

I told Fabian to remove it from the snippet to avoid confusion because when the disabled attribute is present, even if set to true, the value attribute doesn't work, and no value is submitted. This might confuse users, so I told him to just remove it from the snippet.

@tishoyanchev
Copy link
Contributor

What do you mean by the select options not being submitted?

@verena-ifx

When you put the multi-select component inside a

, and select multiple options, and then submit it, the options were not getting submitted, but his is already resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working major major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radio-button, Checkbox and Switch: align to web standards Input components: name attribute is missing
3 participants