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

Refactor vertical scrollbar to only be visible when text content overflows #1438

Merged
merged 4 commits into from
Oct 31, 2022

Conversation

shwanton
Copy link

@shwanton shwanton commented Sep 27, 2022

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

#1366 Added a vertical scrollbar to all Multiline TextInputs. This change caused some visual regression since it was showing scrollbars even if the text content did not overflow the view.

CleanShot 2022-09-26 at 18 12 26

There are also instances where we don't want the vertical scrollbar to be visible. For this edge case we can pass hideVerticalScrollIndicator to always hide the scrollbar.

NOTE #1244 Adds support for disabling scrollview. Once that PR is merged, I will update this to also not show the vertical scrollbar when scrolling is disabled.

Fixes #1368

Changelog

  • Only show the vertical scrollbar if the text content overflows bounds of containing view.
  • Hide the scrollbar when hideVerticalScrollIndicator is passed as prop

[Fixed] [macOS] - Optionally hide the vertical scrollbar & only display when text overflows containing view

Test Plan

Pasting text that overflows container view

CleanShot.2022-09-26.at.18.14.59.mp4

Input has pre-populated value that overflows container view

CleanShot.2022-09-26.at.18.16.41.mp4

Input has pre-populated value that does not overflow container view

CleanShot.2022-09-26.at.18.17.10.mp4

passing hideVerticalScrollIndicator prop as true will always hide the vertical scrollbar

CleanShot.2022-09-26.at.18.19.19.mp4

@shwanton shwanton requested a review from a team as a code owner September 27, 2022 01:22
@shwanton
Copy link
Author

@Saadnajmi Just tested against master & this is ready to be merged.

Copy link

@chiuam chiuam left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for making this change!

@chiuam chiuam merged commit 277129c into microsoft:main Oct 31, 2022
@shwanton shwanton deleted the vertical-scrollbar-refactor branch November 3, 2022 23:20
shwanton added a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
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.

TextInput: make showsVerticalScrollIndicator a configurable prop
3 participants