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.

Sign up for GitHub

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

Jump to bottom

fix(tabs): prevent vertical auto scroll #4592

Closed
wants to merge 12 commits into from
Closed

fix(tabs): prevent vertical auto scroll #4592

wants to merge 12 commits into from

Conversation

Rocss
Copy link
Contributor

@Rocss Rocss commented Jul 9, 2024 •
edited
Loading

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.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 9, 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:

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 9, 2024 •
edited
Loading

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.99 0.94 0.99
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.276 kB 212.139 kB 211.986 kB 🏆
Scripts 56.136 kB 49.776 kB 49.584 kB 🏆
Stylesheet 35.012 kB 30.386 kB 🏆 30.454 kB
Document 6.137 kB 5.341 kB 5.334 kB 🏆
Font 126.991 kB 126.636 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

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 9, 2024 •
edited
Loading

Tachometer results

Chrome

tabs permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 425 kB 103.57ms - 104.72ms - faster ✔
1% - 4%
1.10ms - 4.37ms
branch 413 kB 105.35ms - 108.42ms slower ❌
1% - 4%
1.10ms - 4.37ms		 -

top-nav permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 430 kB 37.28ms - 38.47ms - faster ✔
1% - 5%
0.28ms - 1.96ms
branch 418 kB 38.40ms - 39.59ms slower ❌
1% - 5%
0.28ms - 1.96ms		 -
Firefox

tabs permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 425 kB 203.60ms - 212.88ms - unsure 🔍
-4% - +3%
-7.46ms - +5.30ms
branch 413 kB 204.94ms - 213.70ms unsure 🔍
-3% - +4%
-5.30ms - +7.46ms		 -

top-nav permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 430 kB 91.62ms - 98.78ms - faster ✔
1% - 12%
1.31ms - 12.45ms
branch 418 kB 97.82ms - 106.34ms slower ❌
1% - 13%
1.31ms - 12.45ms		 -

@Rocss Rocss marked this pull request as ready for review July 11, 2024 12:09
@Rocss Rocss requested a review from a team July 11, 2024 12:09
Rajdeepc
Rajdeepc requested changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

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

Good Approach! Can you also add some storybook tests here so that we can surface it up to the VRTs

packages/tabs/src/Tabs.ts Outdated Show resolved Hide resolved
packages/tabs/src/Tabs.ts Show resolved Hide resolved
packages/tabs/src/Tabs.ts Outdated Show resolved Hide resolved
packages/tabs/src/Tabs.ts Outdated Show resolved Hide resolved
@Rocss
Copy link
Contributor Author

Rocss commented Jul 12, 2024

Good Approach! Can you also add some storybook tests here so that we can surface it up to the VRTs

@Rajdeepc there is one VRT for this auto scroll feature in the Tabs Overflow stories, would you like me to add more edge cases to that story?

@Rajdeepc
Copy link
Contributor

Good Approach! Can you also add some storybook tests here so that we can surface it up to the VRTs

@Rajdeepc there is one VRT for this auto scroll feature in the Tabs Overflow stories, would you like me to add more edge cases to that story?

Yes with the one Jian has raised a bug! That would be helpful

@blunteshwar
Copy link
Collaborator

Can you update this to main and also some VRTs are failing.

@Rocss Rocss requested a review from Rajdeepc July 17, 2024 08:11
@Rajdeepc
Copy link
Contributor

Closing this due to commit override, merging here

@Rajdeepc Rajdeepc closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rajdeepc Rajdeepc Awaiting requested review from Rajdeepc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rocss @Rajdeepc @blunteshwar