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

Port over the Header component used across all sites #210

Merged
merged 2 commits into from
Oct 9, 2020

Conversation

winkerVSbecks
Copy link
Collaborator

No description provided.

Copy link
Contributor

@kylesuss kylesuss left a comment

Choose a reason for hiding this comment

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

LGTM. Good call bringing it over since it is being reused. I'm 👍 on pulling it in, assuming it fits w/ Dom's design thinking on how it will be used in the future.

}

export const defaultHeaderContext = {
navBreakpoint: breakpoint * 1.25,
Copy link
Member

Choose a reason for hiding this comment

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

navBreakpoint differs between projects.

What's the thinking behind 1.25 as the multiple? That would yield a breakpoint of 600px * 1.25 = 750px.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does differ. Seems to be based on how many links are in the header. Some have more and need to collapse at a wider width. I'll make 1*breakpoint the default. 1.25 was based on what one of the sites was using.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And to confirm. You can change the value through a prop.

Copy link
Member

Choose a reason for hiding this comment

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

The breakpoint value or the multiple? I typically use the breakpoint from the design system and change the multiple when doing calculations. It's admittedly quite rough.

e.g.,

  • 1 === 600px
  • 1.25 === 750px
  • 1.333 === 800px
  • 1.5 === 900px

Copy link
Collaborator Author

@winkerVSbecks winkerVSbecks Oct 8, 2020

Choose a reason for hiding this comment

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

The multiple. I spotted 1.25 and 1.33 on the other sites. Example usage:

<Header
    logo={<Logo />}
    navBreakpoint={breakpoint*1.33}
/>

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I've never quite understood why we use the multiple -- it feels a bit arbitrary when you implement it in a consumer app. I like the idea of defining common breakpoint sizes a bit more, but is there a benefit I am missing to this multiple strategy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be my preference too. I think the challenge is that those few breakpoints don't work for all the scenarios we have across multiple sites. Something to consider for the next refactor.

@winkerVSbecks winkerVSbecks merged commit 8996a3f into master Oct 9, 2020
@winkerVSbecks winkerVSbecks deleted the header-component branch October 9, 2020 17:49
@winkerVSbecks winkerVSbecks added the enhancement New feature or request label Oct 13, 2020
@kylesuss
Copy link
Contributor

🚀 PR was released in v5.1.27 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants