-
Notifications
You must be signed in to change notification settings - Fork 13.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
refactor(navbar): migrate Bootstrap navbar to AntD menus #14184
Conversation
/testenv up |
@pkdotson Ephemeral environment spinning up at http://34.219.170.233:8080. Credentials are |
/testenv up |
@rusackas Ephemeral environment spinning up at http://54.191.255.143:8080. Credentials are |
A few little visual details remaining:
... there also seem to be an issues with linting that're holding up CI. |
Codecov Report
@@ Coverage Diff @@
## master #14184 +/- ##
==========================================
+ Coverage 77.09% 77.12% +0.02%
==========================================
Files 959 958 -1
Lines 48309 48344 +35
Branches 5661 5676 +15
==========================================
+ Hits 37243 37283 +40
+ Misses 10866 10861 -5
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@pkdotson Ephemeral environment spinning up at http://54.245.29.178:8080. Credentials are |
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.
thank you for the PR. im seeing below
- Nav bar jumpiness
- Inconsistent caret style and position
are those intentional?
Screen.Recording.2021-04-23.at.11.28.21.AM.mov
im okay with the change here. @rusackas |
looks like a bug. caret style and position shouldnt be switching locations |
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.
Looking much better with new changes. Adding some styling questions just to make sure the approach makes sense.
font-weight: ${({ theme }) => theme.typography.weights.bold}; | ||
margin-right: ${({ theme }) => theme.gridUnit * 3}px; | ||
text-align: left; | ||
font-size: 18px; |
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.
font-size: 18px; | |
font-size: 18px; |
Ask @mihir174 about this - the design system includes 16px (l) and 21px (xl) font sizes... perhaps we should use one of these?
display: flex; | ||
align-items: center; | ||
padding: 8px 0; | ||
padding: 14px 0; |
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.
There are a few of these specific-pixel paddings in here. They're probably fine, but maybe @mihir174 would like to see what the next grid unit up or down looks like, particularly in combination with the font size adjustment mentioned above.
} | ||
.navbar-nav { | ||
|
||
.ant-menu-horizontal:not(.ant-menu-dark) > .ant-menu-item { |
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.
Just curious why we need the not(.ant-menu-dark)
bit here... should this not be spaced the same if we did use the dark menu option?
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.
Another override, because the default on the items were to large
top: 51px !important; | ||
border-radius: 0px !important; | ||
@media (max-width: 767px) { | ||
top: 269px !important; |
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 whole block of new styles contains a lot of highly specific pixel measurements. Is this introducing fragility by pinning things in particular locations? I just want to make sure that this doesn't look all broken if something else changes, like the height of a logo or something.
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.212.23.59:8080. Credentials are |
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.
approving as product sign-off
Fixes #13492 |
What do we need to do to get this through? |
@mistercrunch Evan and I were trying to figure out how to fix the overrides that we have to put in the less file. This will most likely end up as a follow up as this is not really a blocker. |
/testenv up |
@rusackas Ephemeral environment spinning up at http://52.11.103.208:8080. Credentials are |
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 do we need to do to get this through?
@mistercrunch Evan and I were trying to figure out how to fix the overrides that we have to put in the less file. This will most likely end up as a follow up as this is not really a blocker.
Indeed @mistercrunch and @pkdotson I've been holding this up over some concerns about styling:
- There are some aspects of this layout that don't quite line up with our design system (font sizes/paddings) and don't use the theme variables from there
- There are a lot of
!important
styles added tosuperset.less
that seem antithetical to our attempts to move away from LESS and into Emotion. - There are some oddly specific pixel values in the styles to nudge things around, which look a bit fragile to me, and would be better served by rethinking how the layout styles flow.
That said, while I see the above concerns as some degree of tech debt, the net benefit of this PR outweighs these concerns. I think this can be merged as is, and we can follow up on these concerns later.
Ephemeral environment shutdown and build artifacts deleted. |
* initial commit * more polish * fix types and remove tests * fix tests, update menu css, add oetc * fix lint and precommit * fix test * update css, address comments * fix lint * update submenu for extra buttons * remove block and lint * fix lint * remove block * adjust margin * test round 2 * test round 3 * about section * src/components/Menu/Menu.test.tsx * remove redundant test * fmore pointed test * fix lint * remove unused css * fix dashboard nav view * update comments * use suggestion * lint-fix * move css, fix dropdown and text * lint * rearchitect main nav component * run lint fix * nit
* initial commit * more polish * fix types and remove tests * fix tests, update menu css, add oetc * fix lint and precommit * fix test * update css, address comments * fix lint * update submenu for extra buttons * remove block and lint * fix lint * remove block * adjust margin * test round 2 * test round 3 * about section * src/components/Menu/Menu.test.tsx * remove redundant test * fmore pointed test * fix lint * remove unused css * fix dashboard nav view * update comments * use suggestion * lint-fix * move css, fix dropdown and text * lint * rearchitect main nav component * run lint fix * nit
* initial commit * more polish * fix types and remove tests * fix tests, update menu css, add oetc * fix lint and precommit * fix test * update css, address comments * fix lint * update submenu for extra buttons * remove block and lint * fix lint * remove block * adjust margin * test round 2 * test round 3 * about section * src/components/Menu/Menu.test.tsx * remove redundant test * fmore pointed test * fix lint * remove unused css * fix dashboard nav view * update comments * use suggestion * lint-fix * move css, fix dropdown and text * lint * rearchitect main nav component * run lint fix * nit
SUMMARY
This pr will migrate the old react-bootstraps navbar into ant-d menus. This pr will also update smaller screen navbar views to more user friendly views.
Changes affect the main menu and submenu components
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before
https://user-images.githubusercontent.com/17326228/115632043-cbfef580-a2bb-11eb-8a49-e75b422bec85.mov
after
Screen.Recording.2021-04-21.at.3.45.50.PM.mov
TEST PLAN
Will update test and manually check that each submenu looks similar to existing specs.
ADDITIONAL INFORMATION