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

feat: chip implementation for uikit #111

Merged
merged 12 commits into from
Mar 7, 2023
Merged

Conversation

daniel-dumortier
Copy link
Contributor

@daniel-dumortier daniel-dumortier commented Jan 18, 2023

Changes description

This PR adds the Chip component for UIKit version.

Context

Implements the UIKit part of issue #20

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
  • Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
  • Check your code additions will fail neither code linting checks.
  • I have reviewed the submitted code.
  • I have tested on an iPhone device/simulator.
  • I have tested on an iPad device/simulator.
  • If it includes design changes, please ask for a review design-system-core-team-design GitHub team.

Does this introduce a breaking change?

  • No

Screenshots

iPhone

Simulator Screen Shot - iPhone 8 - 2023-01-18 at 14 05 04
Simulator Screen Shot - iPhone 8 - 2023-01-18 at 14 04 58
Simulator Screen Shot - iPhone SE (3rd generation) - 2023-03-03 at 16 34 17

iPad

Simulator Screen Shot - iPad (9th generation) - 2023-01-18 at 14 05 57
Simulator Screen Shot - iPad (9th generation) - 2023-03-03 at 16 35 12

Other information

@daniel-dumortier daniel-dumortier requested review from a team as code owners January 18, 2023 13:07
@lauthieb lauthieb linked an issue Jan 19, 2023 that may be closed by this pull request
@lauthieb lauthieb removed a link to an issue Jan 19, 2023
Sources/VitaminUIKit/Components/Chip/README.md Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Chip/README.md Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Chip/README.md Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Chip/README.md Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Chip/README.md Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Chip/VitaminChip.swift Outdated Show resolved Hide resolved
Sources/VitaminUIKit/Components/Chip/VitaminChip.swift Outdated Show resolved Hide resolved
@florentlotthepro florentlotthepro requested a review from a team January 20, 2023 09:41
@daniel-dumortier daniel-dumortier added the Ready for Design Review The showcase is ready for design review label Feb 6, 2023
@SimonLeclercq
Copy link

@daniel-dumortier, I have some issues with the current chips:
1 - filter variant display a tag on iOS sample, but it's not required on this platform.
2 - I'm not able to close an input-chips. I don't know if it's due to the sample or if it's not provided.

Thanks ☺️

@daniel-dumortier daniel-dumortier requested a review from a team February 8, 2023 11:08
@daniel-dumortier
Copy link
Contributor Author

daniel-dumortier commented Feb 8, 2023

@SimonLeclercq
Regarding filter variant:
Do you mean a badge(and not a tag) ?
Honestly, there were two sources, and I did not know which one was the good one:

I decided to implement the more complex one ;).
Before reverting my code, can you confirm that Figma is the source of truth (and eventually update ZeroHeight) ?

Regarding the input variant :
All variant can be provided with an action callback that will be launched when user taps the chip.
I thought it was a better idea to let the responsibility of the dismissal of the input chip within this callback to the app developer.
But I you think it should be an intrinsic behavior of this component, that's ok for me, i'll dismiss the chip ans execute the callback just after.

@SimonLeclercq
Copy link

@daniel-dumortier yes I mean badge, excuse me.

Well noted regarding the two sources of truth, I'll take this point. Issue here: Decathlon/vitamin-design#191

I'll take a look a callback and give you more detail as soon as possible 😀

Copy link

@SimonLeclercq SimonLeclercq left a comment

Choose a reason for hiding this comment

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

@daniel-dumortier, discuss with team, the 'tag' inside filter variant is a miss (copy from web, but not available for mobile.). In this way, can you delete it?

Apart from that, it's good for me 🚀

@daniel-dumortier
Copy link
Contributor Author

@SimonLeclercq Done, the badge has been removed, you can find the last showcase on slack

Sources/VitaminUIKit/Components/Chip/README.md Outdated Show resolved Hide resolved
Co-authored-by: Florent LOTTHÉ <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Mar 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@daniel-dumortier daniel-dumortier merged commit 22016d7 into main Mar 7, 2023
@daniel-dumortier daniel-dumortier deleted the feature/uikit_chip branch March 7, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚀 New feature or request Ready for Design Review The showcase is ready for design review UIKit
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants