-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Block activation of primary buttons on background screens #8802
Conversation
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.
Changes look good, just have a comment withNavigationFocus. Can we add tests to make sure we don't fail the error boundary as well?
export default Button; | ||
export default compose( | ||
withNavigationFallback, | ||
withNavigationFocus, |
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.
What does the withNavigationFocus do here?
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.
Oh, I forgot to push the final commit. Good catch.
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'm still not seeing how it is used, could you explain please?
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 HOC gives is a new prop called isFocused
and we are using that to determine if the button is visible on the currently focused/active screen. This way all the buttons on the backgrounded screens will not trigge with enter
press.
these are covered with StoryBook tests. Other than this, I don't QA will be able to test Error UI unless there is a app crash. |
f6d9041
to
4f273cc
Compare
Tests well on web, running into issue building storybook but seems unrelated to this change |
Link needs to be updated Also can you add a SC for the storybook build? Running into an issue with |
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.
Approved pending storybook screenshots
@thienlnam I reported the issue about storybook https://expensify.slack.com/archives/C01GTK53T8Q/p1651081332743029. We will have to apply this fix https://expensify.slack.com/archives/C01GTK53T8Q/p1651082528577659?thread_ts=1651081332.743029&cid=C01GTK53T8Q to fix the stories. This is out of scope for this PR. |
Updated. |
Cool cool 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. |
🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
Details
Fixed Issues
$ #7623
Tests
Contributor (PR Author) Checklist
main
### Fixed Issues
section abovesrc/languages/*
files (if applicable)Styling.md
) for all style edits I madeSTYLE.md
)Avatar
, I verified the components usingAvatar
are working as expected)main
branch)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectionsrc/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
property(i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)QA Steps
Screenshots
Mobile Web
Desktop | Web
screen-2022-03-22_23.38.43.mp4
iOS
Android
Storybook