-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add collapser component #98
Conversation
… to elements rendered
…o open row on page load. Update css for dark mode, use theme variables
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.
Had a few suggestions for you. Nice work on this! Would you mind also posting a screenshot of the component in the PR description? Thanks!
src/content/kitchen-sink.mdx
Outdated
japaneseVersion: '' | ||
--- | ||
|
||
<Collapser> |
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.
Taking a look at how this component is put together, it looks like each CollapserRow
is also wrapped by its own Collapser
component. Because of that, does it even make sense to wrap these rows from a user perspective?
I could however see this being useful for making sure a "collapser group" has the proper spacing in between each collapser row. What if we rename these slightly?
Collapser
-> CollapserGroup
CollapserRow
-> Collapser
The thinking is this: I should be able to use a single Collapser
on its own. If I only have a single collapser, I shouldn't need to embed it in a CollapserGroup
(hence the name). If I want multiple Collapser
s next to each other (like this example), then I'd wrap it in a CollapserGroup
to make sure there is proper space between them.
Thoughts on this idea?
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.
my thinking was eventually in the <Collapser />
component we may add things like a button to open all <CollapserRow>
rows on click. buuut that and maybe some styling on the container were the only use cases i could think of, really.
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.
Thinking on this more, I think I get what you're puttin' down. Will work on that today, may ping you for some input :)
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 probably should have provided an example to better explain this. Ideally I'd be able to use the collapser 1 of 2 ways:
// on its own
<Collapser title="My title">
Stuff
</Collapser>
// When I have multiple
<CollapserGroup>
<Collapser title="My title">
Stuff
</Collapser>
<Collapser title="My title 2">
Stuff 2
</Collapser>
</CollapserGroup>
CollapserGroup
would be responsible here for adding the spacing between each of the Collapser
s. Does that help clarify?
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.
we may add things like a button to open all rows on click
That should still be possible with this design if we put it in the CollapserGroup
. It may not make sense to show a "open all" button anyways if there is a single collapser.
src/components/CollapserRow.js
Outdated
}; | ||
|
||
return ( | ||
<Collapser> |
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 see each row is wrapped by its on collapser. Do we want this? From a component design perspective, components tend to scale a bit better if components don't add their own surrounding whitespace (the callout components are the one exception to this). We want the parent components to layout their children so that they can space them accordingly. This ensures you can use a component in multiple contexts without having to rejigger the spacing each time you do.
Because of that, I think this surrounding <Collapser>
should actually be removed and instead have the the consumer of the API use it.
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.
oh dang. did not mean for each row to be inside a <Collapser />
. Will remove that so all row components are inside one <Collapser />
src/components/CollapserRow.js
Outdated
|
||
const CollapserRow = ({ title, id, openByDefault, children }) => { | ||
const [isOpen, toggleOpen] = useState(openByDefault); | ||
const [setHeight, setHeightState] = useState(isOpen ? 'auto' : '0px'); |
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.
[nit] What about naming this height
instead of setHeight
. Then your setHeightState
could become setHeight
, which to me reads a bit more natural.
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.
yep, totally agree.
src/components/CollapserRow.js
Outdated
<h5 | ||
id={id} | ||
css={css` | ||
font-family: 'Open Sans', sans-serif; |
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.
We shouldn't need this since the font family is already defined site-wide.
src/components/CollapserRow.js
Outdated
text-align: left; | ||
margin-top: 0; | ||
margin-bottom: 0; | ||
font-size: 16px; |
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.
Should we try and maintain the same defaults for an h5
as we do the rest of the site? In that case, you'd likely want to remove the font-size
, and font-weight
properties. I think the margin-top: 0
and margin-bottom: 0
make sense here since you're trying to remove it for the purposes of this component. Do we need to override the default size and weight for the h5
?
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.
when i removed the font-size i feel like it made the text just a bit too small. 16px or 1rem seemed about right. i get what you're saying though. would be more ideal if we didn't override default heading style.
we could always use another tag though too. i used <h5>
because the current site primarily uses <h2>
and <h3>
, and on rare occasion <h4>
.
src/components/CollapserRow.js
Outdated
css={css` | ||
font-family: 'Open Sans', sans-serif; | ||
font-weight: 600; | ||
font-size: 14px; |
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.
You can remove this since its a duplicate of line 67
src/components/CollapserRow.js
Outdated
font-family: 'Open Sans', sans-serif; | ||
font-weight: 600; | ||
font-size: 14px; | ||
text-align: left; |
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.
This is the default, and it doesn't look like anything overrides it, so you should be safe to remove this.
src/components/CollapserRow.js
Outdated
display: flex; | ||
flex-direction: column; | ||
border-radius: 3px; | ||
margin-bottom: 8px; |
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.
Seeing as this div
is the container of the whole component, I think we should let the parent component define the space between the rows rather than have this define it. Doing so would help us use this component in more places without having to rejigger the space each time.
src/components/CollapserRow.js
Outdated
</button> | ||
<div | ||
ref={content} | ||
style={{ maxHeight: `${setHeight}` }} |
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.
Since setHeight
is already a string, wrapping it in a string doesn't do much here. You can just set the value directly:
style={{ maxHeight: setHeight }}
That being said, one really cool thing about emotion is that you can interpolate dynamic values right into the styles. Rather than using an inline style here (which translates to the style
property on the element in the DOM), you can add this to the css
prop below:
css={css`
overflow: hidden;
transition: max-height 0.6s ease;
max-height: ${setHeight};
`}
@jerelmiller Just pushed changes based on your feedback to this branch. One question I had was how to add margin if we now have a standalone |
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.
This looks 💯 . Thanks so much for making those updates!!!
Closes #72
Add collapser component that's similar in look to NR1 Help Center style. You can pass an
id
string so the row is directly linkable via a hash in URL. You can specify if the row should be open by default on page load.Kept it fairly basic. See Notes for things we may want to eventually add.
Notes
react-spring
for better animating the height of the hidden/shown content.f
(when user finds on page).Screenshot