-
Notifications
You must be signed in to change notification settings - Fork 3.5k
17033 - migrated HeaderView to PressableWithoutFeedback #19958
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
17033 - migrated HeaderView to PressableWithoutFeedback #19958
Conversation
|
@Skalakid bump to merge main! |
aimane-chnaif
left a comment
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.
@Skalakid please remove Podfile.lock changes which is irrelevant.
|
@Skalakid job3 failing. please merge main and let's see if it came from this PR |
| style={[styles.flexRow, styles.alignItemsCenter, styles.flex1]} | ||
| disabled={isTaskReport} | ||
| accessibilityLabel={title} | ||
| accessibilityRole="button" |
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.
NAB, can we add these roles to CONST.js?
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.
That will be done in another PR. I think #20251.
So it makes sense to put this PR on hold a bit.
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.
Actually I think we can just merge this. From #20251 (comment), it makes sense to replace all of them in one PR as we merged many PRs with hardcoded strings already.
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.
That makes sense, thank you
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
@aimane-chnaif @grgia I just fixed the job3, the problem was with not passing |
grgia
left a comment
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!
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.27-1 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.27-7 🚀
|
Details
Fixed Issues
$ 17033
PROPOSAL: 17033
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
mWeb-chrome.mov
Mobile Web - Safari
mWeb-safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov