Skip to content
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

Fix sidebar rendering on page load #33

Closed
wants to merge 1 commit into from
Closed

Fix sidebar rendering on page load #33

wants to merge 1 commit into from

Conversation

brks
Copy link

@brks brks commented Mar 21, 2019

Along with the issue concerning the sizing of the GitHub icon on the homepage raised in #28, there was an issue with the rendering of the sidebar that is explained in detail in #32. This pull request should prevent that issue from occurring and when paired with the fix in #31, prevent any kind of rendering issues on page load.

@brks brks requested a review from a team March 21, 2019 23:36
@brks brks requested a review from a team as a code owner March 21, 2019 23:36
@lucasjohnston
Copy link
Contributor

👋Thanks for your contribution!
At Monzo we try to avoid using css class names anywhere unless we absolutely have to, so we would approach this slightly differently.

Instead of this:

<Sidebar className={isSidebarVisible ? 'visible' : null}>
    ...
</Sidebar>
display: none;
&.visible {
    display: flex;
}

We would approach it like this:

<Sidebar visible={isSidebarVisible}>
    ...
</Sidebar>
display: none;
${props =>
    props.visible &&
    css`
      display: flex;
    `}

However, in any case, I'm actually going to be moving all of the logic out of react and into css media queries next week, so I don't think we'll be merging this PR - apologies about that. Thank you for offering your help and time on this ❤️

@brks
Copy link
Author

brks commented Mar 29, 2019

@lucasjohnston no worries, thank you for the heads up! Happy to try and help out even though it's not the solution you were looking for 😄

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.

2 participants