-
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
Move articles from Account Settings sub-category into Settings parent… #42867
Conversation
@ 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] |
@rushatgabhane - As with the category rename I submitted a bit earlier, I did this article move using github.dev. Feel free to let me know where I can do better in the process since I'm documenting all this for my team to use github.dev moving forward. |
A preview of your ExpensifyHelp changes have been deployed to https://152e8103.helpdot.pages.dev ⚡️ |
I reviewed the changes and maybe things are okay but I bet I could have taken care of what I'm seeing when I made the PR. What I'm seeing is the articles are correctly moved to be visible under the Settings parent category, but they also still exist under the Account Settings sub-category. I had planned to create a PR to delete the Account Settings sub-category anyway, so when I do that I think it'll delete all the articles that are still visible under the sub-category in the preview link. |
@strepanier03 this PR is not quite what we want. I've attached a video of what you'll need to do, and then commit the changes in a new PR.
Screen.Recording.2024-05-31.at.02.07.22.mov |
we should do everything in one PR |
Got it, thanks so much Rushat. I'll make the new PR today. |
Closing this PR since I was able to follow Rushat's great instructions and create a new PR here - #43879 |
Per the request here I've been tasked with moving all the articles currently under Classic > Settings > Account Settings to live under Classic > Settings instead.
I will also request a redirect for all the articles and then delete Account Settings once the articles are moved.
Details
Fixed Issues
$
PROPOSAL:
Tests
Offline 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
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop