-
Notifications
You must be signed in to change notification settings - Fork 65
improve props doc for tag comp. #504
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I chatted with Mike about this yesterday. When the Tag component was initially released the Box props were intentionally not included in the prop types or documentation. This was so that the Tag component could maintain a certain look and feel without implementations drifting too far away from that. Mike also mentioned "There's a lot of things that use Box as part of it's implementation, but the more we rely on it and document the props it does make harder to vary the implementation going forward" Even though Tag can currently be passed Box props as soon as we start documenting that we have locked ourselves into a contract that we will continue to support that. For now I don't think we should add Box props to the documentation for Tag. |
Since Tag is built with Box it will inherit the BoxPropTypes so there is no need to add those propTypes anyways however I thought the TS should be there when there is a request coming from the community who is using Color Tag is very common among other component libraries and other worlds so I don't think that we should limit our grommet users in what they can do... However, I understand it not being in our design system documentation. |
As a follow up to Brittany's comment, I also noticed that we have documented similar use cases on other components as highlighted already so we should rather be consistent in our documentation to enhance better user experience and predictability of how we build and communicate about our component library. We might need to revisit this and agree on a more unified approach across all our components. Having said that, I think background should be included for Tag component in the documentation since the use case outside HPE might usually require this common request. I understand Jessica and Mike's point as well but I believe we might need to rethink the amount of balance we want to achive and maintain over time between grommet use case for HPE and the opensource community. |
@jcfilben and I discussed this and at this point I think we should have To not lock ourselves into a contract we can leave off all boxProps as a whole. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
No description provided.