Skip to content

Remove horizontal scrollview indicator padding when empty #1635

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
Merged
merged 16 commits into from
Jan 25, 2023
Merged

Remove horizontal scrollview indicator padding when empty #1635

merged 16 commits into from
Jan 25, 2023

Conversation

shwanton
Copy link

@shwanton shwanton commented Jan 13, 2023 •
edited
Loading

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

  • Horizontal scrollviews with no content/height have a 15px padding to account for the horizontal scrollbar which is enabled on init.

Empty horizontal scrollview. Padding in red

CleanShot 2023-01-12 at 15 48 42

  • Split up the scroll indicator example into horizontal/vertical so we can better test each scenario
CleanShot.2023-01-17.at.10.10.04.mp4

Changelog

[macOS] [FIXED] - Remove horizontal scrollview indicator padding when empty

Test Plan

When scrollview is empty (content removed), the scrollview is not visible

CleanShot.2023-01-12.at.22.11.03.mp4

shwanton
shwanton commented Jan 13, 2023
View reviewed changes
shwanton
shwanton commented Jan 13, 2023
View reviewed changes
analysis-bot
analysis-bot suggested changes Jan 13, 2023
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

@ghost ghost removed the Needs: Author Feedback label Jan 13, 2023
@analysis-bot
Copy link

analysis-bot commented Jan 13, 2023 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 2430d1f
Branch: main

@shwanton shwanton marked this pull request as ready for review January 13, 2023 06:51
@shwanton shwanton requested a review from a team as a code owner January 13, 2023 06:51
analysis-bot
analysis-bot suggested changes Jan 13, 2023
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot
analysis-bot suggested changes Jan 13, 2023
View reviewed changes
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

@ghost ghost removed the Needs: Author Feedback label Jan 13, 2023
@Saadnajmi
Copy link
Collaborator

Can some of the refactoring of tests be pushed upstream so we can instead cherry-pick it to RN-macOS? Might help with reducing diffs :)

@shwanton
Copy link
Author

Can some of the refactoring of tests be pushed upstream so we can instead cherry-pick it to RN-macOS? Might help with reducing diffs :)

On iOS, the vertical/horizontal scrollbar indicator overlays can both be toggled/displayed at the same time.

CleanShot.2023-01-17.at.08.48.21.mp4

On macOS, the horizontal scroll indicator will only be visible if the horizontal prop is present. The ScrollIndicatorExample does not have the horizontal prop.

https://github.com/microsoft/react-native-macos/blob/main/packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js#L711-L719

I think we should fork them & show both horizontal & vertical examples separately since the behavior is slightly different relative to the horizontal prop. I can remove the iOS ScrollIndicatorExample if it's confusing to have all 3 in the list.

@Saadnajmi
Copy link
Collaborator

@shwanton / @chiuam I personally prefer keeping the iOS example as it makes future merges easier. Is the macOS dependency on the "horizontal" prop necessary, or something that can change? Those are my two cents, I'll defer to @chiuam who I see already approved =)

@chiuam
Copy link

chiuam commented Jan 17, 2023

@shwanton / @chiuam I personally prefer keeping the iOS example as it makes future merges easier. Is the macOS dependency on the "horizontal" prop necessary, or something that can change? Those are my two cents, I'll defer to @chiuam who I see already approved =)

I also prefer keeping the iOS example to showcase the behavioral difference from macOS. Agreed with @Saadnajmi that we should upstream any applicable changes as much as possible to reduce diffs - is horizontal a macOS specific prop?

@shwanton
Copy link
Author

Is the macOS dependency on the "horizontal" prop necessary, or something that can change?

horizontal is available for all platforms. It turns out that even w/ this prop set you can trigger both scroll indicators if the ScrollView ContentView is wider than its parent view.

CleanShot 2023-01-17 at 14 29 41

I'll revert the split out examples & try to upstream a better example to core.

Saadnajmi
Saadnajmi reviewed Jan 21, 2023
View reviewed changes
@shwanton shwanton closed this Jan 24, 2023
@shwanton shwanton deleted the fix-horizontal-scrollview-default-padding branch January 24, 2023 23:46
@shwanton shwanton restored the fix-horizontal-scrollview-default-padding branch January 24, 2023 23:51
@shwanton shwanton reopened this Jan 24, 2023
@Saadnajmi Saadnajmi merged commit 21aeabf into microsoft:main Jan 25, 2023
@shwanton shwanton deleted the fix-horizontal-scrollview-default-padding branch January 25, 2023 22:10
shwanton added a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
@shwanton
[RN][macOS] Scrollview overlay and indicator behavior
70dd1b7 
Summary:
## Sync OS changes for ScrollBar

[[ScrollView] Don't always adjust content view padding for both scrollers microsoft#1132](microsoft#1132)

[ScrollView - Respect the shows<Horizontal | Vertical>ScrollIndicator props on macOS microsoft#1135](microsoft#1135)

[Remove horizontal scrollview indicator padding when empty microsoft#1635](microsoft#1635)

[Add ScrollView Indicator Overlay style microsoft#1658](microsoft#1658)

NOTE: This will override the "Always" user setting and force the scrollview to use the overlay style instead

{F850963270}


Test Plan:
|ScrollView Overlay Style|
|https://pxl.cl/2qp0D|

|ScrollView works correctly|
|https://pxl.cl/2qpd1|

Reviewers: chpurrer, lefever

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D42637723
shwanton added a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
@shwanton @chiuam @Saadnajmi
Remove horizontal scrollview indicator padding when empty (microsoft#…
3621b47 
…1635)

* Don't compute additional horizontal padding for indicator if the content view is empty

This was causing default padding when height was 0

* Switch the horizontal/vertical scroller size

These were both returning the same value so masked the bug

* Split up inset examples to be vertical/horizontal

* Refactor scroll indicator examples into macOS specific examples

* Add back deleted example

* Use ContentSize to measure scrollview content height

* Remove unused var

* Fix lint

* Remove ios ScrollIndicatorExample from rn tester

* Revert broken out scroll indicator changes to `ScrollViewExample.js`

Try to upstream these to RN core instead

* Use correct macOS tags

Co-authored-by: Shawn Dempsey <shawndempsey@fb.com>
Co-authored-by: chiuam <67026167+chiuam@users.noreply.github.com>
Co-authored-by: Saad Najmi <sanajmi@microsoft.com>
shwanton added a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
@shwanton
[RN][macOS] Scrollview overlay and indicator behavior
ee5f53e 
Summary:
## Sync OS changes for ScrollBar

[[ScrollView] Don't always adjust content view padding for both scrollers microsoft#1132](microsoft#1132)

[ScrollView - Respect the shows<Horizontal | Vertical>ScrollIndicator props on macOS microsoft#1135](microsoft#1135)

[Remove horizontal scrollview indicator padding when empty microsoft#1635](microsoft#1635)

[Add ScrollView Indicator Overlay style microsoft#1658](microsoft#1658)

NOTE: This will override the "Always" user setting and force the scrollview to use the overlay style instead

{F850963270}


Test Plan:
|ScrollView Overlay Style|
|https://pxl.cl/2qp0D|

|ScrollView works correctly|
|https://pxl.cl/2qpd1|

Reviewers: chpurrer, lefever

Reviewed By: chpurrer

Subscribers: generatedunixname89002005327315

Differential Revision: https://phabricator.intern.facebook.com/D42637723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Saadnajmi Saadnajmi Saadnajmi left review comments

+2 more reviewers

@analysis-bot analysis-bot analysis-bot requested changes

@chiuam chiuam chiuam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chiuam chiuam

Labels

Partner: Facebook

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shwanton @analysis-bot @Saadnajmi @chiuam