open-amdocs / webrix

Powerful building blocks for React-based web applications
https://webrix.amdocs.com
Apache License 2.0
432 stars 32 forks source link

Scrollable performance refactor #70

Closed yairEO closed 2 years ago

yairEO commented 2 years ago

Performance improvements:

see bug: bugs.chromium.org/p/chromium/issues/detail?id=1266517

codecov[bot] commented 2 years ago

Codecov Report

Merging #70 (cbbb465) into master (cf0e9e8) will increase coverage by 0.04%. The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   95.39%   95.44%   +0.04%     
==========================================
  Files          61       61              
  Lines         847      856       +9     
  Branches       27       29       +2     
==========================================
+ Hits          808      817       +9     
  Misses         33       33              
  Partials        6        6              
Impacted Files Coverage Δ
src/components/Scrollable/Scrollable.jsx 95.31% <92.00%> (+0.39%) :arrow_up:
...onents/HorizontalScrollbar/HorizontalScrollbar.jsx 94.11% <100.00%> (+0.78%) :arrow_up:
...components/VerticalScrollbar/VerticalScrollbar.jsx 94.11% <100.00%> (+0.78%) :arrow_up:

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 cf0e9e8...cbbb465. Read the comment docs.

ykadosh commented 2 years ago

Don't forget to use the string fix() in one of your commits to trigger a release. Also, we need a PR in webrix-docs with the updated version and documentation updates if applicable.

yairEO commented 2 years ago

@ykadosh - if I put a fix in my commit, does Github knows my user have the permissions to trigger a build?

Thanks for reminding me to also make a PR in webrix-docs which documents this fix.

Initially I thought not documenting this change because it is something which was done specifically for Amdocs needs and requires an optional prop to <Scrollable cssVarsOnTracks={true}>, hoping Google will fix their bug this year:

https://bugs.chromium.org/p/chromium/issues/detail?id=1266517

It took some time to find a good-enough name for this prop

ykadosh commented 2 years ago

@yairEO the fix() commit message will only trigger a build once the PR is merged to master, so you don't have to worry about permissions. You can have as many fix() commits as you'd like, and it will only create a single patch version for all of them.

Regarding the documentation - i'm OK with not documenting this, as long as the default behavior is maintained (i.e. CSS variables on the parent by default). But you will need to create a PR there anyway, for updating the version in the package (which updates the version in the site header too).