Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Conversation

@BeniCheni
Copy link
Contributor

@BeniCheni BeniCheni commented Mar 3, 2019

While checking out responsive styles, observed a style bug that below 380px, the header nav items would display in an odd layout, which would cause UX issue like the hamburger menu wouldn’t open. Tried out this PR to:

  • use emotion for styles in header's JSX so that it can support existing style code and hook into media query
  • use padding 0 up to 380px, where the original problemetcal breakpoint started to occur
  • update test snapshot.
  • update code format with "format" script.
Before Fix

screen shot 2019-03-02 at 9 33 39 pm

With Fix @ 320px breakpoint (common like the legacy iPhone 5)

screen shot 2019-03-02 at 9 57 10 pm

@BeniCheni BeniCheni marked this pull request as ready for review March 3, 2019 02:58
@BeniCheni BeniCheni changed the title Header style small width Adjust Header's padding below 380px Mar 3, 2019
@BeniCheni BeniCheni changed the title Adjust Header's padding below 380px style: Adjust Header's padding below 380px Mar 3, 2019
@sagirk
Copy link
Contributor

sagirk commented Mar 3, 2019

@BeniCheni There's a lot of noise in this PR 'coz of the formatting changes. Can we please have only your changes, so its easier to review?

FYI, there's a pending PR which reformats the codebase with prettier/tslint. It is waiting on #157 to land before it gets merged.

@BeniCheni
Copy link
Contributor Author

@sagirk, reverted the code format change for easy review. Glad #97 would take care of code format update in bulk. Thanks.

Copy link
Contributor

@sagirk sagirk left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@sagirk
Copy link
Contributor

sagirk commented Mar 3, 2019

/gcbrun

Copy link
Member

@ZYSzys ZYSzys left a comment

Choose a reason for hiding this comment

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

LGTM👍

Thanks for your effort.

@ZYSzys ZYSzys merged commit b18d235 into nodejs:master Mar 4, 2019
@ZYSzys
Copy link
Member

ZYSzys commented Mar 4, 2019

Thank you !

@BeniCheni BeniCheni deleted the header-style-small-width branch March 4, 2019 02:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants