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

add scroll padding to multiline and labeled text input #8137

Merged
merged 8 commits into from
Mar 25, 2022
Merged

add scroll padding to multiline and labeled text input #8137

merged 8 commits into from
Mar 25, 2022

Conversation

mollfpr
Copy link
Contributor

@mollfpr mollfpr commented Mar 14, 2022 •
edited
Loading

Details

Adding scroll-padding will give offset scroll to text input and the value will not be hidden by the label.

Fixed Issues

$ #7729

Tests

  1. Go to Settings page > Workspace page > Manage members page
  2. Click invite button
  3. Have more than 5 lines in personal message, and put the cursor at end
  4. Use up arrows and go to the first line
  5. Verify the top line of text is visible
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I verified the PR has a small number of commits behind main
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests & veryfy they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
    • I followed the JSDocs style guidelines (in STYLE.md)
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I corroborated the UI performance was not affected (the performance is the same than main branch)
  • If I created a new component I verified that a similar component doesn't exist in the codebase

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

QA Steps

  1. Go to Settings page > Workspace page > Manage members page
  2. Click invite button
  3. Have more than 5 lines in personal message, and put the cursor at end
  4. Use up arrows and go to the first line
  5. Verify the top line of text is visible
  • Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-03-24.at.01.45.54.mov

mWeb

Screen.Recording.2022-03-24.at.01.46.51.mov

Desktop

Screen.Recording.2022-03-24.at.01.47.38.mov

iOS

Screen.Recording.2022-03-24.at.01.50.36.mov

Android

Screen.Recording.2022-03-24.at.01.51.35.mov

@mollfpr mollfpr requested a review from a team as a code owner March 14, 2022 13:42
@MelvinBot MelvinBot requested review from Julesssss and parasharrajat and removed request for a team March 14, 2022 13:42
@parasharrajat
Copy link
Member

Sorry, I forgot to ask about it in the proposal. Does this issue not affect native platforms? I checked and Native platforms also have this issue. You can to scroll manually to see the top line. Does this solution works on native?

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 14, 2022

@parasharrajat
Unfortunately this does not fix the issue in native.

I think is very rare someone using keyboard external and use the arrow key to reproduce the issue. Only my opinion😅

@parasharrajat
Copy link
Member

I think is very rare someone using keyboard external and use the arrow key to reproduce the issue. Only my opinionsweat_smile

Yeah! you are correct. I meant to ask will it be a problem with the native platform?

parasharrajat
parasharrajat suggested changes Mar 14, 2022
View reviewed changes
src/styles/styles.js Show resolved Hide resolved
@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 16, 2022

@parasharrajat I put the style to TextInput/index.js so it will not be applied to the native component.

parasharrajat
parasharrajat suggested changes Mar 16, 2022
View reviewed changes
src/styles/styles.js Outdated Show resolved Hide resolved
src/components/TextInput/index.js Outdated Show resolved Hide resolved
mollfpr and others added 2 commits March 16, 2022 19:00
@mollfpr @parasharrajat
Update src/styles/styles.js
f37058c 
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
@mollfpr @parasharrajat
Update src/components/TextInput/index.js
0a729da 
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
parasharrajat
parasharrajat suggested changes Mar 16, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Ok this is not working on firefox.

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 16, 2022

@parasharrajat what firefox version do you use?

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 16, 2022

I tested with Firefox 98.0.1 it seems to work.

Screen.Recording.2022-03-16.at.19.13.15.mov

@parasharrajat
Copy link
Member

parasharrajat commented Mar 16, 2022 •
edited
Loading

I don't know why it is not working on my FF. Technically, It should.

FF developer 96.0b10 (64-bit

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 16, 2022

@parasharrajat Please try again, I added overflow. I also tried in Firefox Developer Edition 99.0b4 (64-bit) is working.

@parasharrajat
Copy link
Member

parasharrajat commented Mar 16, 2022 •
edited
Loading

I am still not able to 😞 . Asked for help https://expensify.slack.com/archives/C01GTK53T8Q/p1647450300876539.

@Julesssss Could you please try this on Firefox?

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 18, 2022

No takers on slack 😅

@mollfpr mollfpr requested a review from parasharrajat March 18, 2022 03:52
@parasharrajat
Copy link
Member

Yeah, I just pinged on the thread. There must be something that we are missing if this is working for you. What do you suggest? I will try incognito.

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 18, 2022

Also try to take a screenshot from inspect element, maybe we have a clue from there.

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 18, 2022

I just try on FF developer 96.0b10 (64-bit) it's also working 🤔

Screen.Recording.2022-03-18.at.11.02.52.mov

parasharrajat
parasharrajat suggested changes Mar 22, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

I was able to test it on Firefox and looks good. Just a few comments.

src/styles/styles.js Outdated
@@ -698,6 +698,11 @@ const styles = {
borderRadius: variables.componentBorderRadiusNormal,
},

textInputMultiline: {
scrollPadding: '23px 0 0 0',
overflowY: 'auto',
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it’s not, there’s a warning from FF because not detect a scroll of the input and probably the offset scroll not detected.

It’s just some attempt to make it work on your end before.

It’s safe to remove the overflow.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove it

src/styles/styles.js
@@ -698,6 +698,11 @@ const styles = {
borderRadius: variables.componentBorderRadiusNormal,
},

textInputMultiline: {
scrollPadding: '23px 0 0 0',
Copy link
Member

@parasharrajat parasharrajat Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

        scrollPadding: '23px 0 0 0 0',

I think we only need top padding. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry ignore this, I am on drugs... Lol.

@mollfpr mollfpr requested a review from parasharrajat March 23, 2022 11:22
parasharrajat
parasharrajat approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM,

cc: @Julesssss

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

🎀 👀 🎀 C+ reviewed

parasharrajat
parasharrajat suggested changes Mar 23, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Please attach videos/screenshots for rest of the platforms.

@mollfpr

This comment was marked as duplicate.

Sign in to view
@parasharrajat
Copy link
Member

parasharrajat commented Mar 23, 2022 •
edited
Loading

I meant on the PR description 🙂

@mollfpr
Copy link
Contributor Author

mollfpr commented Mar 23, 2022

Ahh sorry 😅

parasharrajat
parasharrajat approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM

cc: @Julesssss

@Julesssss Julesssss merged commit 9c50db7 into Expensify:main Mar 25, 2022
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@mollfpr mollfpr deleted the fix-7729 branch March 29, 2022 10:26
@OSBotify OSBotify mentioned this pull request Mar 30, 2022
Closed
47 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.47-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@botify botify mentioned this pull request Apr 5, 2022
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Apr 5, 2022

🚀 Deployed to production by @roryabraham in version: 1.1.49-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat approved these changes

@Julesssss Julesssss Julesssss approved these changes

Assignees

@mollfpr mollfpr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mollfpr @parasharrajat @OSBotify @Julesssss