-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
17013 - migrate OptionRow to PressableWithFeedback #19612
Conversation
@arosiclair Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@Skalakid make sure to assign yourself when creating PRs. Some of our automation depends on that. @robertKozik could you do the checklist for this as well? |
Sure, I'll do it later today |
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.
Please change these two thing and It should be GTG
src/components/OptionRow.js
Outdated
accessibilityLabel={this.props.option.text} | ||
accessibilityRole="button" | ||
hoverDimmingValue={1} | ||
pressDimmingValue={0.8} |
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.
the current default value for press opacity is the same
Line 143 in 79ddcab
pressDimValue: 0.8, |
src/components/OptionRow.js
Outdated
@@ -174,6 +174,10 @@ class OptionRow extends Component { | |||
this.props.isDisabled && styles.cursorDisabled, |
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.
I know it wasn't changed in your PR, but this style is now redundant as PressableWithFeedback
is managing it itself. Thus this line can be removed.
Reviewer Checklist
Screenshots/VideosWebdesktop.-.web.movMobile Web - Chromeandroid.-.web.movMobile Web - SafariiOS.-.web.movDesktopdesktop.-.native.moviOSiOS.-.native.movAndroidandroid.-.native.mov |
@Skalakid I've found one inconsistency. With the new implementation if the optionRow is in keyboard focus and then hovered the blue border is dismissed. In the old one it stayed
CC. @arosiclair OLDold.movNEWnew.mov |
@arosiclair I've added a fix for the on-focus styling problem, that @robertKozik mentioned above. To prevent the blue border from disappearing I changed the focus style to be the same as the hover style. With this on-focused style looks more standardized, what do you think about it? Nagranie.z.ekranu.2023-05-31.o.12.54.53.mov |
Hmm I think preferably we just apply both the focus border and hover highlight. It looks like |
The blue border is rendered by a web browser, I believe. We are adding styles to |
Fair enough let's move forward with that! |
Looks good 🚀 Reviewer checklist is now filled @arosiclair all yours |
✋ 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 staging by https://github.com/arosiclair in version: 1.3.26-0 🚀
|
This PR QA is failing because of this issue #20513 |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.26-4 🚀
|
This might be a regression - #20585 @Skalakid @arosiclair could you take a peek? |
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.
These changes resulted in the following regression #19612. We probably forgot to test this case.
@@ -176,6 +176,7 @@ class OptionRow extends Component { | |||
accessibilityRole="button" | |||
hoverDimmingValue={1} | |||
hoverStyle={this.props.hoverStyle} | |||
focusStyle={this.props.hoverStyle} |
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.
This caused a regression #20585
Details
Fixed Issues
$ 17013
PROPOSAL: 17013
Tests
This component is used in many places, you can test it by checking the list of users that
Steps that I took:
Members
OptionRow
component. Verify if these buttons work fineOffline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
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)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
web-mobile-chrome.mov
Mobile Web - Safari
web-mobile-safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov