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(tabs): prevent vertical auto scroll #4613

Merged
merged 6 commits into from
Jul 19, 2024
Merged

fix(tabs): prevent vertical auto scroll #4613

merged 6 commits into from
Jul 19, 2024

Conversation

Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Jul 19, 2024

Description

Possible fix for #4590

The problem with scrollIntoView is that we can not specify the direction of scroll, therefore it scrolls vertically as well, and the function is called all the time, even if you don't actually need to scroll.

This implementation provides an alternative, with validations before scrolling, but unfortunately a lot of checks needed (RTL / LTR, take into account the space between the Tabs and the arrows).

Related issue(s)

Motivation and context

Replaces scrollIntoView with calculations based of scroll offsets.
A follow-up of #4032.

How has this been tested?

  • Test case 1 All VRTs should pass, as we want to keep this change transparent for the consumers.
  • Test case 2
    1. Go here
    2. Using the selected control, enter the number of a tab which is not visible
    3. The component should automatically scroll to it
  • Test case3
    1. Go here
    2. Using the inspector add to the .container class a margin-top of 1000px, or whatever value is needed so that the tabs are no longer into view
    3. Using the selected control, change the selected value, you can choose a number from 1 to 20
    4. The page does not vertically scroll
  • Test case 4
    1. Go here
    2. Change from LTR to RTL
    3. Using the selected control, enter the number of a tab which is not visible
    4. The component should automatically scroll to it

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@Rajdeepc Rajdeepc requested a review from a team July 19, 2024 06:53
Copy link

github-actions bot commented Jul 19, 2024

Branch preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Copy link

github-actions bot commented Jul 19, 2024

Tachometer results

Chrome

tabs permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 425 kB 102.81ms - 104.22ms - faster ✔
1% - 3%
0.66ms - 2.75ms
branch 413 kB 104.45ms - 105.99ms slower ❌
1% - 3%
0.66ms - 2.75ms
-

top-nav permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 430 kB 36.96ms - 38.02ms - faster ✔
1% - 5%
0.33ms - 1.89ms
branch 418 kB 38.02ms - 39.17ms slower ❌
1% - 5%
0.33ms - 1.89ms
-
Firefox

tabs permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 425 kB 205.02ms - 213.46ms - unsure 🔍
-5% - +2%
-10.09ms - +3.41ms
branch 413 kB 207.31ms - 217.85ms unsure 🔍
-2% - +5%
-3.41ms - +10.09ms
-

top-nav permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 430 kB 91.10ms - 99.86ms - unsure 🔍
-9% - +3%
-8.95ms - +2.95ms
branch 418 kB 94.44ms - 102.52ms unsure 🔍
-3% - +9%
-2.95ms - +8.95ms
-

Copy link

github-actions bot commented Jul 19, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.99 0.94
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 224.232 kB 211.882 kB 211.747 kB 🏆
Scripts 56.079 kB 49.619 kB 49.496 kB 🏆
Stylesheet 35.156 kB 30.32 kB 30.301 kB 🏆
Document 6.095 kB 5.339 kB 5.336 kB 🏆
Font 126.902 kB 126.604 kB 🏆 126.614 kB

Request Count

Category Latest Main Branch
Total 48 48 48
Scripts 40 40 40
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

blunteshwar
blunteshwar previously approved these changes Jul 19, 2024
Copy link
Collaborator

@blunteshwar blunteshwar left a comment

Choose a reason for hiding this comment

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

LGTM

@Rajdeepc Rajdeepc merged commit e1ef097 into main Jul 19, 2024
58 checks passed
@Rajdeepc Rajdeepc deleted the rocss/4590-fix branch July 19, 2024 08:25
Rajdeepc added a commit that referenced this pull request Aug 2, 2024
* chore(tabs): updated tabs overflow to fix scroll into view for horizontal scroll

* chore(tabs): updated golden image cache

---------

Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
Co-authored-by: Rajdeep Chandra <rajdeepchandra@rajdeeps-mbp-2.macromedia.com>
Rajdeepc added a commit that referenced this pull request Aug 2, 2024
* chore(tabs): updated tabs overflow to fix scroll into view for horizontal scroll

* chore(tabs): updated golden image cache

---------

Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
Co-authored-by: Rajdeep Chandra <rajdeepchandra@rajdeeps-mbp-2.macromedia.com>
Rajdeepc added a commit that referenced this pull request Aug 2, 2024
* chore(tabs): updated tabs overflow to fix scroll into view for horizontal scroll

* chore(tabs): updated golden image cache

---------

Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
Co-authored-by: Rajdeep Chandra <rajdeepchandra@rajdeeps-mbp-2.macromedia.com>
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.

3 participants