Fix: Sidebar nav bugs by kunaljain0212 · Pull Request #1447 · nodejs/nodejs.dev

kunaljain0212 · 2021-06-18T08:13:01Z

Description

Fixed the navigation issue for about tab.
Added a fixed with to sidenav keeping the responsiveness in mind.
Fixed the height issue for sidenav

To discuss:
The className of <main> tag was not consistent across different pages of the tab because of which it was causing spacing issues and also when I tried to decrease the width to check for responsiveness of the website the article section was overlapping with the sidenav. To overcome that I gave all the <main> tags a consistent className of "grid-container", now that issues is fixed. I am attaching images for reference.

Before:

After:

Related Issues

Fixes #1446

kunaljain0212 · 2021-06-18T08:13:39Z

/preview

github-actions · 2021-06-18T08:16:37Z

Please find a preview at: https://staging.nodejs.dev/1447/

rodion-arr · 2021-06-18T08:38:21Z

There is no way close about sidebar on mobile:

kunaljain0212 · 2021-06-18T09:11:40Z

Thankyou for the review @rodion-arr . I have fixed both the issues, could you please review them once. Seems like the width of sidenav was changing because of different classNames of <main> tag. Since now they are consistent width: auto is working fine. Also the position of nav in responsive view from top had a slight error because of which the button was hidden now it is visible.

kunaljain0212 · 2021-06-18T09:12:08Z

/preview

github-actions · 2021-06-18T09:14:50Z

Please find a preview at: https://staging.nodejs.dev/1447/

manishprivet

It seems like the package-lock.json was modified while you did npm install. We are already using dependabot for updating dependencies in lockfile, so can you please revert this.

Also, it seems like you need to update the snapshots for the tests to pass.

Thanks

… sidebarNav

codecov-commenter · 2021-06-18T16:13:09Z

Codecov Report

Merging #1447 (311b315) into main (8d49750) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1447   +/-   ##
=======================================
  Coverage   85.98%   85.98%           
=======================================
  Files          73       73           
  Lines         792      792           
  Branches      216      216           
=======================================
  Hits          681      681           
  Misses        111      111

Impacted Files	Coverage Δ
src/components/AboutPageSideNavBar/index.tsx	`90.90% <ø> (ø)`
src/pages/about.tsx	`100.00% <ø> (ø)`
src/pages/governance.tsx	`100.00% <ø> (ø)`
src/pages/privacy.tsx	`100.00% <ø> (ø)`
src/pages/releases.tsx	`100.00% <ø> (ø)`
src/pages/resources.tsx	`100.00% <ø> (ø)`
src/pages/security.tsx	`100.00% <ø> (ø)`
src/pages/trademark.tsx	`100.00% <ø> (ø)`
src/pages/working-groups.tsx	`100.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d49750...311b315. Read the comment docs.

github-actions · 2021-06-18T16:16:32Z

Please find a preview at: https://staging.nodejs.dev/1447/

kunaljain0212 · 2021-06-18T17:06:34Z

Added sidenav bar to releases page as well, as mentioned in the updated description of the issue #1447

github-actions · 2021-06-18T18:31:00Z

Please find a preview at: https://staging.nodejs.dev/1447/

github-actions · 2021-06-19T03:03:56Z

Please find a preview at: https://staging.nodejs.dev/1447/

manishprivet · 2021-06-19T03:22:57Z

LGTM

manishprivet · 2021-06-19T03:50:17Z

@benhalverson The mobile menu, is that supposed to be like the learn page, or this is fine for now?

benhalverson

I think it's ok for now. Ideally it should match the /learn mobile menu.

manishprivet · 2021-06-19T06:45:47Z

I think we should wait for #1297 to be merged so that this PR finishes the about page once and for all

… sidebarNav

manishprivet · 2021-06-19T18:58:52Z

      overflow-y: scroll;
      position: fixed;
-      top: var(--nav-height);
+      top: calc(var(--nav-height) * 2);


I think you can remove this now 🙂

Right, removing it.

manishprivet · 2021-06-19T19:01:54Z

  /* Needed since overflow: none will disable position: sticky on children */
  clip-path: polygon(0 0, 100% 0, 100% 100%, 0 100%);
  background-color: var(--color-fill-side-nav);
-  height: calc(100vh - var(--nav-height));


These changes are seemingly breaking the /learn page on mobile (https://staging.nodejs.dev/1447/learn)

Can you keep the styling changes limited to about.scss only

#1448 is taking care of the style issues

The cross button is not visible in the deployed version (https://nodejs.dev/learn), which restricts the user from closing the sidenav in mobile view.

#1448 handled that as well.

Great, thanks.

Here the pending thing, is you need to revert these lines to their original state.

Reverted back the above-mentioned changes and changes for extender class as both of these are being handled in #1448

github-actions · 2021-06-20T04:18:59Z

Please find a preview at: https://staging.nodejs.dev/1447/

Fix: Sidebar nav bugs

3905153

rodion-arr suggested changes Jun 18, 2021

View reviewed changes

Comment thread src/styles/about.scss Outdated

Fix: Closing button and Mobile view

43d855f

benhalverson added the create-preview Generate preview on staging.nodejs.dev label Jun 18, 2021

Merge branch 'main' into sidebarNav

774d4a9

github-actions Bot removed the create-preview Generate preview on staging.nodejs.dev label Jun 18, 2021

manishprivet suggested changes Jun 18, 2021

View reviewed changes

Comment thread src/styles/learn.scss Outdated

nodejs deleted a comment from github-actions Bot Jun 18, 2021

kunaljain0212 added 3 commits June 18, 2021 21:21

Fix: Updated snapshots and made compatible for all browsers

2083c7e

Merge branch 'sidebarNav' of github.com:kunaljain0212/nodejs.dev into…

cfd0d19

… sidebarNav

Fix: Updated snapshots

f6abf88

manishprivet added the create-preview Generate preview on staging.nodejs.dev label Jun 18, 2021

github-actions Bot removed the create-preview Generate preview on staging.nodejs.dev label Jun 18, 2021

Fix: Added sidenav bar to releases page

3a0550f

manishprivet added the create-preview Generate preview on staging.nodejs.dev label Jun 18, 2021

github-actions Bot removed the create-preview Generate preview on staging.nodejs.dev label Jun 18, 2021

kunaljain0212 added 2 commits June 19, 2021 00:05

Fix: Fixed className for working groups page

78d39e5

Fix: Updated snapshot for working groups

2ce2165

manishprivet added the create-preview Generate preview on staging.nodejs.dev label Jun 19, 2021

github-actions Bot removed the create-preview Generate preview on staging.nodejs.dev label Jun 19, 2021

manishprivet approved these changes Jun 19, 2021

View reviewed changes

benhalverson approved these changes Jun 19, 2021

View reviewed changes

MrJithil reviewed Jun 19, 2021

View reviewed changes

Comment thread src/styles/learn.scss

MrJithil mentioned this pull request Jun 19, 2021

fix: side nav full width in mobile view. #1448

Merged

manishprivet and others added 4 commits June 19, 2021 23:13

Merge branch 'main' into sidebarNav

0bbca97

Fix: Extender class about.scss

2d0fe37

Merge branch 'sidebarNav' of github.com:kunaljain0212/nodejs.dev into…

139ad06

… sidebarNav

Fix: Extender class and Security page

507da48

manishprivet suggested changes Jun 19, 2021

View reviewed changes

kunaljain0212 added 3 commits June 20, 2021 00:40

Removing unnecessary styling

a8c5b5a

Reverting changes

c84276e

Fixed linting issues

311b315

benhalverson added the create-preview Generate preview on staging.nodejs.dev label Jun 20, 2021

github-actions Bot removed the create-preview Generate preview on staging.nodejs.dev label Jun 20, 2021

kunaljain0212 requested a review from manishprivet June 20, 2021 12:57

manishprivet approved these changes Jun 20, 2021

View reviewed changes

benhalverson merged commit 2e5c834 into nodejs:main Jun 20, 2021

Conversation

kunaljain0212 commented Jun 18, 2021

Description

Related Issues

Uh oh!

kunaljain0212 commented Jun 18, 2021

Uh oh!

github-actions Bot commented Jun 18, 2021

Uh oh!

Uh oh!

rodion-arr commented Jun 18, 2021

Uh oh!

kunaljain0212 commented Jun 18, 2021

Uh oh!

kunaljain0212 commented Jun 18, 2021

Uh oh!

github-actions Bot commented Jun 18, 2021

Uh oh!

manishprivet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jun 18, 2021

Uh oh!

kunaljain0212 commented Jun 18, 2021

Uh oh!

github-actions Bot commented Jun 18, 2021

Uh oh!

github-actions Bot commented Jun 19, 2021

Uh oh!

manishprivet commented Jun 19, 2021

Uh oh!

manishprivet commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benhalverson left a comment

Choose a reason for hiding this comment

Uh oh!

manishprivet commented Jun 19, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

manishprivet left a comment •

edited

Loading

codecov-commenter commented Jun 18, 2021 •

edited

Loading

manishprivet commented Jun 19, 2021 •

edited

Loading