Skip to content

(PDS-566) Add prop to Tag component to show/hide button #360

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

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

jilliankeenan
Copy link
Contributor

@jilliankeenan jilliankeenan commented Nov 5, 2020

This PR adds an additional prop hideRemoveButton to the tag component - for cases where you just want to display a static tag without giving the ability to the user to remove it.

Screenshot 2020-11-05 at 17 17 25

This PR adds an additional prop hideRemoveButton to the tag component - for cases where you just want to display a static tag without giving the ability to the user to remove it
@jilliankeenan jilliankeenan requested a review from a team as a code owner November 5, 2020 17:14
Copy link
Contributor

@eputnam eputnam left a comment

Choose a reason for hiding this comment

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

LGTM.

Had we considered making the prop static instead of hideRemoveButton? I'm just thinking about this being a future construct across components that means it can't be acted on. I guess that can be addressed further down the road and i do appreciate a prop that says exactly what it's doing.

@jilliankeenan
Copy link
Contributor Author

jilliankeenan commented Nov 6, 2020

LGTM.

Had we considered making the prop static instead of hideRemoveButton? I'm just thinking about this being a future construct across components that means it can't be acted on. I guess that can be addressed further down the road and i do appreciate a prop that says exactly what it's doing.

Yea I felt it was more obvious what the prop was doing by naming it hideRemoveButton but you've made a really good point for example in the SidePanel I implemented a similar functionality where the user can show/hide the close button. Perhaps if we adopted static as the prop name for these cases design system wide it would make more sense for each individual component in this case.

@jilliankeenan jilliankeenan merged commit d7b0ad3 into puppetlabs:development Nov 9, 2020
This was referenced Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants