search

grommet / grommet-site

Site for Grommet v2
Apache License 2.0
32 stars 64 forks source link

improve props doc for tag comp. #504

Closed Sulaymon333 closed 7 months ago

jcfilben commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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.