Fix scrollbar layout shift on all pages
Needs RevisionPublic
Actions

Authored by Chenani-MohamedAmine on Wed, Mar 4, 01:01.

Details

Reviewers

ieli

Maniphest Tasks

T2250: Add route transition animations

Summary

Ref T2250

Diff Detail

Repository

rSP ServPulse

Lint

Lint Passed

Unit

No Test Coverage

Branch

T2250-route-transitions

Build Status

Buildable 6408
Build 6692: arc lint + arc unit

Event Timeline

Chenani-MohamedAmine requested review of this revision.Wed, Mar 4, 01:01

Chenani-MohamedAmine created this revision.

Harbormaster completed remote builds in B6408: Diff 10361.Wed, Mar 4, 01:02

dereckson added a reviewer: ieli.Wed, Mar 4, 06:18

Per https://cbea.ms/git-commit/ titles of your commit messages should be much shorter.

You can consider a global title then use current info as summary:

[New shorter title]

Add fade transition on route changes.

Fix scrollbar layout shift on IncidentHistory loading state.

In D3992#61982, @dereckson wrote:
Per https://cbea.ms/git-commit/ titles of your commit messages should be much shorter.

You can consider a global title then use current info as summary:
[New shorter title]

Add fade transition on route changes.

Fix scrollbar layout shift on IncidentHistory loading state.

Is there a way to have reusable templates stored somewhere? I'd be interesting only if it add no additional complexity to the flow and is centralized — a Phabricator built-in way to store these or have them ready to use would be acceptable.

ieli retitled this revision from Add fade transition on route changes and fix scrollbar layout shift on IncidentHistory loading state. to Fix scrollbar layout shift on all pages.Wed, Mar 4, 13:18

This is out of scope and uses the wrong approach. Please rollback all changes and start over. I've renamed the differential for better clarity.

Basically, your changes are:

Out of scope: You added tests (App.test.js) which are unnecessary
Wrong solution: min-h-[60vh] on the loading spinner is a band-aid that doesn't fix the root cause
Missing the actual fix: The scrollbar layout shift fix is completely missing from staged changes

Research how major companies solve the scrollbar layout shift problem in Vue/Tailwind applications. Look for:

Industry-standard CSS solutions for preventing layout shift when scrollbars appear/disappear
How to properly integrate this with Tailwind CSS
Where the fix should be placed (component styles vs global styles vs Tailwind configuration)

Do not use band-aid solutions like forcing minimum heights. Fix the actual root cause.

Important: This is not the first time that I've had to run the app locally to discover that changes were not tested at all. Work was just pushed and left for me to review.

Making changes that are supposed to be visual and pushing code where I just open the app and find that visually it is not present is not a good habit to have.

If this happens again I'll be forced to refuse to review anymore.

This revision now requires changes to proceed.Wed, Mar 4, 13:29