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(header): scrolling behaviour #1252

Open
wants to merge 17 commits into
base: next
Choose a base branch
from
Open

Conversation

mivaylo
Copy link
Contributor

@mivaylo mivaylo commented Feb 16, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: CDE-1659

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

BREAKING CHANGES: Removed scroll from body when header nav and subnav are opened, wrapped content of header nav inside nav HTML element

This PR addresses 2 scrolling issues:

  1. Body is scrollable when you open the header nav or subnav and user can scroll the background (which is dimmed). This is in contrast to the behavior that we provide with the modal components, where the background is not scrollable when you open the modal and again scrollable once you close it (if of course the body had been scrollable before opening the modal)

  2. Header nav and subnav are not scrollable and if they contain a big list of menu items they are cut and not visible

Example of first issue:
screen-capture (1).webm

Example of second issue (list is cut and not fully visible as there is no scrollbar):
image

This PR is opened to next branch because it contains breaking changes

Breaking changes are:
-removed scroller from the body (as well as the added ones to the .header-nav and .subnav)
-the list of nav items in the .header-nav are now wrapped in a new nav HTML element. This is needed as the .header-nav is a modal like DOM part (it has contents and X close button that dims the background, and the X close button is created with Javascript code, so the X close button should not be in the same level as the nav links)

Demo of the changes in the PR:
demo changes.webm

Copy link
Contributor

github-actions bot commented Feb 16, 2024

👋 @mivaylo,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, view a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal Clarity Support space

Thank you,

🤖 Clarity Release Bot

@mivaylo mivaylo changed the base branch from next to main February 16, 2024 16:15
@mivaylo mivaylo marked this pull request as draft February 16, 2024 16:15
@mivaylo mivaylo changed the base branch from main to next February 16, 2024 16:15
@mivaylo mivaylo changed the title fix(header): remove obsolete background scroll fix(header): scrolling behaviour Feb 22, 2024
Copy link
Contributor

This PR introduces visual changes: e1f85a8
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git e1f85a8f7e037911ac23ca521935975179765cc0
git cherry-pick e1f85a8f7e037911ac23ca521935975179765cc0
git push

Copy link
Contributor

This PR introduces visual changes: 60af79d
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git 60af79d9e6e9741b0c71f8e6e3363cba3d997877
git cherry-pick 60af79d9e6e9741b0c71f8e6e3363cba3d997877
git push

Copy link
Contributor

This PR introduces visual changes: d34e825
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git d34e8255f3aa85f4f37d49b41d4a4bafa00f8c2c
git cherry-pick d34e8255f3aa85f4f37d49b41d4a4bafa00f8c2c
git push

Copy link
Contributor

This PR introduces visual changes: aaa5927
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git aaa5927c3f9273bc62c5cfa93c41ce89cec5fb41
git cherry-pick aaa5927c3f9273bc62c5cfa93c41ce89cec5fb41
git push

Copy link
Contributor

This PR introduces visual changes: 1c804d6
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git 1c804d6b55ab50dbb34bdb515a8a97a77a8ad16f
git cherry-pick 1c804d6b55ab50dbb34bdb515a8a97a77a8ad16f
git push

@mivaylo mivaylo marked this pull request as ready for review February 27, 2024 08:21
Copy link
Contributor

This PR introduces visual changes: a966d82
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git a966d82a6847465320ba9deae2befa081c5bf5a9
git cherry-pick a966d82a6847465320ba9deae2befa081c5bf5a9
git push

Copy link
Contributor

This PR introduces visual changes: 7cd827f
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout cde-1659-header-scrolling
git fetch https://github.com/vmware-clarity/ng-clarity.git 7cd827f6f1660b3f30cecde4bf89a27295c68ee1
git cherry-pick 7cd827f6f1660b3f30cecde4bf89a27295c68ee1
git push

@mivaylo mivaylo marked this pull request as ready for review June 13, 2024 10:28
@mivaylo mivaylo requested review from valentin-mladenov and a team June 17, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants