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(apps): add status chips #872

Merged
merged 3 commits into from
May 24, 2024
Merged

feat(apps): add status chips #872

merged 3 commits into from
May 24, 2024

Conversation

swouf
Copy link
Contributor

@swouf swouf commented May 24, 2024

No description provided.

@swouf swouf added feature New feature or request apps 📱 Relates to apps labels May 24, 2024
@swouf swouf self-assigned this May 24, 2024
Copy link

sonarcloud bot commented May 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@swouf swouf marked this pull request as ready for review May 24, 2024 10:35
Copy link
Contributor

@ReidyT ReidyT left a comment

Choose a reason for hiding this comment

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

Nice PR ! It looks good to me 🚀. I just have a question: do you think it could be interesting to have a StatusChip component who have the same props as the others, but with a colour and an icon in addition ? This could allows to reuse this component inside Required, Saved and SubmittedChip component and reducing the little code duplication. Maybe it is not what we want, it is just a question 😉.

Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

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

@ReidyT @swouf I agree, we could have a single component that exposes a variant prop that could take the required | submitted | saved values and then simply switch on them in the component. The structure could be the same and you only have to switch on the color and icon properties. You can put them in a dictionary that indexes over the custom chip variant.

But as Thibault has said, do this only if re-use is important.

@swouf
Copy link
Contributor Author

swouf commented May 24, 2024

@ReidyT @spaenleh, thanks for the recommendations. I agree with your suggestion but I wonder if we will add features that differ a lot for each type of chip. As I don't know that currently, I will merge this PR as is but turn your comments into an issue. Thanks again.

@swouf swouf added this pull request to the merge queue May 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2024
@spaenleh spaenleh added this pull request to the merge queue May 24, 2024
Merged via the queue into main with commit b913d79 May 24, 2024
3 checks passed
@spaenleh spaenleh deleted the common-comp-survey-apps branch May 27, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apps 📱 Relates to apps feature New feature or request v4.19.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants