-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix Input is lost upon opening search page #8535
Conversation
I'm not sure why Marc was requested for a review in a draft PR. (I'm guessing it's because I'm modifying Navigation related files) |
Unrelated console error |
}, | ||
}, | ||
cardStyle, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
observation: This code is mostly the same with the non native version of the file
suggestion: If we need a more specific style passed on one platform then have getCardStyle()
return different things depending on the platform rather than create two very similar versions of the same code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call
Pretty much that. 😄 I am keeping tabs on a few areas of interest to me. Mostly so I can stay current on the code but also to offer assistance with any areas that might be confusing. |
That's interesting. Gotta love github to make this possible and so simple. @Luke9389 all yours! |
checkin this out now... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Tests well. Thanks
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
Updated title to remind to pay on 5/20 if no regressions |
d'oh.. this is the PR and not the actual issue :) |
Details
All details here: #6069 (comment)
Gist: Autofocus on TextInput breaks the animation because the browser scrolls to the TextInput!
Making the card's
position: fixed
solves this problem. However, mobile doesn't support position of type fixed. So a create a native file is created which uses the old implementation.Fixed Issues
$ #6069
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Screenshots
Web
Screen.Recording.2022-04-10.at.9.55.26.PM.mov
Mobile Web - Safari
Screen.Recording.2022-04-10.at.10.24.14.PM.mov
Mobile Web - Chrome (Android)
screen-20220410-223315.mp4
Desktop
Screen.Recording.2022-04-10.at.10.35.55.PM.mov
iOS
Screen.Recording.2022-04-10.at.10.17.09.PM.mov
Android
screen-20220410-223315.mp4