-
Notifications
You must be signed in to change notification settings - Fork 54
Do / Don't Component #63
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
This pull request is automatically deployed with Now. Latest deployment for this branch: https://doctocat-git-do-dont.primer.now.sh |
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.
Awesome 🔥 Just left a couple suggestions
theme/src/components/do-dont.js
Outdated
Do | ||
</Text> | ||
</Flex> | ||
<Box as='img' src={src} width="100%" height="auto"/> |
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.
What if the API for Do/Dont looked something like this instead:
-<Do src="#">
- Blah blah blah
- </Do>
+ <Do caption="Blah blah blah">
+ <img src="#" width="100%" />
+ </Do>
This would make the component more flexible. I can see it being useful to be able to render more than just img
s in the Do/Don't component.
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.
I generally don't love passing in text content as props as it feels pretty awkward 🤔Especially since these captions have the potential to be long-ish in length. The other alternative would be to allow users to pass in both the img
and Caption
as children, but we have to apply the width
and height
properties to the img
tag which means we'd need to do some cloning and editing of children which I generally try to avoid. I think it's fine to leave this minimal for now and if it starts causing issues we can rethink the API in the future
theme/src/components/do-dont.js
Outdated
|
||
export const DoDontWrapper = ({children}) => { | ||
return ( | ||
<Flex flexDirection={['column', 'row', 'row']}> |
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.
I'm not sure we need this component. In the interest of having a "minimal api surface area," I think we should just show an example of how to wrap the Do
and Dont
components with a Flex
component in the documentation.
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.
💡 Using the Grid
component (instead of Flex
) in the documentation example would probably be cleaner:
<Grid gridTemplateColumns={['1fr', '1fr', '1fr 1fr']} gridGap={3}>
<Do>...</Do>
<Dont>...</Dont>
</Grid>
This would allow us to remove the margin-right from the Do
and Dont
components.
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.
ok!
docs/content/components/dodont.mdx
Outdated
title: Do/Don't | ||
--- | ||
|
||
The Do / Don't set of components is used for providing good & bad examples within documentation. These examples appear side by side on desktop, and vertically stacked on mobile. |
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.
The Do / Don't set of components is used for providing good & bad examples within documentation. These examples appear side by side on desktop, and vertically stacked on mobile. | |
The `Do` and `Dont` components are used for providing good and bad examples within documentation. |
^ I don't think they necessarily have to always be side by side on desktop. Here's an example, where they're stacked on desktop: https://primer.style/design/ui-patterns/messaging#full-width
@emplums I think it would also be helpful to include some documentation on how to import the Do/Dont components in the MDX file:
|
Co-Authored-By: Cole Bemis <[email protected]>
Co-Authored-By: Cole Bemis <[email protected]>
Hi, I know this is already merged in order to meet training deadline, but this came up in yesterday's show and tell and I wanted to share some feedback. For the documentation are goal is to make writing documentation as low friction as possible. We know that some content owners will likely make updates in the GitHub web UI rather than cloning, and are comfortable with mardkown, but not familiar with react. So we we design the api of the docs, we should be aiming for ease of use, which translates in places to making stronger assumptions and abstractions than we might do otherwise for Primer Components. I think @emplums was heading down the right track with a wrapper, because the markup is going to look a little alien to others, and once you've imported one component, you can import another. The wrapper should account for the components showing side-by-side on desktop, and stacked on mobile. <DoDont>
<Do src="url">
text caption
</Do>
<Dont src="url">
text caption
</Dont>
</Do I also don't think we should account for using the I know this locks us into something that is harder to undo later, but in this case I think it's the right call in order to make writing docs as easy as possible for the widest audience. |
This PR adds the Do / Don't example components. Three components are added:
DoDontWrapper
(name is kinda weird, suggestions welcome): wraps theDo
andDont
examples and provides layout stylesDo
provides a green check icon and layout stylesDont
provides a redX
icon and layout styles