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.

Sign up for GitHub

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

Jump to bottom

Several fixes in the Chip #1943

Merged
merged 15 commits into from
Apr 15, 2024
Merged

Several fixes in the Chip #1943

merged 15 commits into from
Apr 15, 2024

Conversation

raquelarrojo
Copy link
Collaborator

@raquelarrojo raquelarrojo commented Apr 10, 2024 •
edited
Loading

Checklist
(Check off all the items before submitting)

  • Build process is done without errors. All tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
Several fixes in the chip component:

  • Disabled state only affects to the icon with action, not to the entire component.
  • Several types fixed:
    - disabled is only allowed when onClickPrefix or onClickSuffix exist.
    - It is not possible to add both prefixIcon and suffixIcon with action, only one of them with action can be added.
    - It is not possible to add both prefixIcon and suffixIcon (without action), only one of them can be added.
    - It is possible to add an icon with action and an icon without. For example: prefix with action but suffix without it.

Snapshots
Before the disabled affected the whole component:
image

Now disabled only affects the icon with action:

image

Documentation will be updated in a separate PR after this one is merged.

Closes #1873 and #1707

@raquelarrojo raquelarrojo linked an issue Apr 10, 2024 that may be closed by this pull request
disabled in chip should only affect to the icon #1707
Open
@raquelarrojo raquelarrojo marked this pull request as ready for review April 10, 2024 14:32
@raquelarrojo raquelarrojo changed the title Action fixes in the Chip Several fixes in the Chip Apr 10, 2024
@Mil4n0r Mil4n0r self-requested a review April 11, 2024 09:49
@Mil4n0r Mil4n0r self-assigned this Apr 11, 2024
Mil4n0r
Mil4n0r requested changes Apr 11, 2024
View reviewed changes
lib/src/chip/types.ts Outdated Show resolved Hide resolved
lib/src/chip/types.ts Outdated Show resolved Hide resolved
lib/src/chip/types.ts Outdated Show resolved Hide resolved
lib/src/chip/Chip.stories.tsx Show resolved Hide resolved
lib/src/chip/Chip.stories.tsx Show resolved Hide resolved
Mil4n0r
Mil4n0r requested changes Apr 15, 2024
View reviewed changes
lib/src/chip/types.ts Outdated Show resolved Hide resolved
Mil4n0r
Mil4n0r previously approved these changes Apr 15, 2024
View reviewed changes
Copy link
Collaborator

@Mil4n0r Mil4n0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Good job!

@Mil4n0r Mil4n0r merged commit b18ba60 into master Apr 15, 2024
4 checks passed
@Mil4n0r Mil4n0r deleted the rarrojolopez/chip-fixes branch April 15, 2024 14:42
@raquelarrojo raquelarrojo mentioned this pull request Apr 18, 2024
Merged
@raquelarrojo raquelarrojo restored the rarrojolopez/chip-fixes branch April 18, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mil4n0r Mil4n0r Mil4n0r approved these changes

Assignees

@Mil4n0r Mil4n0r

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chip must not have both prefix and suffix actions disabled in chip should only affect to the icon
2 participants
@raquelarrojo @Mil4n0r