Skip to content

Conversation

Sulaymon333
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@britt6612 britt6612 left a comment

Choose a reason for hiding this comment

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

LGTM

@jcfilben
Copy link
Collaborator

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.

@britt6612
Copy link
Collaborator

britt6612 commented Jan 31, 2024

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 background and they are using TS within their project I would think we should support them. I agree there are lots of areas that we have ...rest that we do not document however I also do not see the harm if someone from the community is specifically asking how to do something with a component to add it. Guess Im trying to understand why it is fine to document in Avatar, Card, Footer, Header, Page etc that they are built with Box, but not Tag

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.

@sulaymon-tajudeen-hpe
Copy link
Contributor

sulaymon-tajudeen-hpe commented Jan 31, 2024

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 background and they are using TS within their project I would think we should support them. I agree there are lots of areas that we have ...rest that we do not document however I also do not see the harm if someone from the community is specifically asking how to do something with a component to add it. Guess Im trying to understand why it is fine to document in Avatar, Card, Footer, Header, Page etc that they are built with Box, but not Tag

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.

@britt6612
Copy link
Collaborator

@jcfilben and I discussed this and at this point I think we should have background in the docs since this was a question from a community member and we should strive to have our docs clear so they can achieve what they need in their products.

To not lock ourselves into a contract we can leave off all boxProps as a whole.

Copy link
Collaborator

@britt6612 britt6612 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@jcfilben jcfilben 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!

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.

4 participants