-
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
Feat/push page ws name #30733
Feat/push page ws name #30733
Conversation
@aimane-chnaif 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] |
50be2ef
to
e109a14
Compare
@b4s36t4 conflicts |
@aimane-chnaif Please check now. Updated with main branch. |
src/ONYXKEYS.ts
Outdated
@@ -333,6 +333,7 @@ const ONYXKEYS = { | |||
REPORT_PHYSICAL_CARD_FORM: 'requestPhysicalCardForm', | |||
REPORT_PHYSICAL_CARD_FORM_DRAFT: 'requestPhysicalCardFormDraft', | |||
REPORT_VIRTUAL_CARD_FRAUD: 'reportVirtualCardFraudForm', | |||
WORKSPACE_EDIT_NAME: 'workspaceEditNameDraft', |
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 is in wrong position. Check other Onyx key pairs
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.
@aimane-chnaif Please check now, I have updated the position.
@b4s36t4 also fix conflict |
@aimane-chnaif PR is ready without conflicts & your requested change. Please check. |
@aimane-chnaif Friendly bump on PR Review. |
@aimane-chnaif You checked in only on browser right?! |
Tested on iOS as well. Seems happening on all platforms |
@aimane-chnaif Can you please verify now? I have tested in all platforms seems working fine. Pushed a fix for the styling issue with currency selections. |
@aimane-chnaif Updated the code, please check. |
@shawnborton As you requested, I did the changes. as per profile page - Default margin is between menus (no extra) and an extra 16px of margin top is there from WSAvatar to the menu items. |
So sorry guys, been sick for the past few days, couldn't able to do the work. Thanks for the support :) |
@b4s36t4 please fix conflict again |
@b4s36t4 bump ^ |
@aimane-chnaif please check updated the code. Sorry for the delay :) |
@b4s36t4 another conflict |
@aimane-chnaif sorry I'm outside now. Will update the code by evening. |
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.
Also fix lint after merge
@aimane-chnaif fixed, please check :) |
@b4s36t4 please resolve conflict again. will complete review as soon as you've done |
@b4s36t4 please merge main. I will complete review during weekend |
@aimane-chnaif Done.! |
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.
After pull main, lint will fail. All new files should be written in TS
@b4s36t4 bump |
@b4s36t4 please close this PR |
Details
Adds a Push to page method for WS screen, removes Save Button in main WS settings.
Fixed Issues
$ #29455
PROPOSAL: #29455 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above.
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
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
Android: Native
android.mp4
Android: mWeb Chrome
chrome.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
safari.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4