-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Update manage-travel-member-roles.md #54308
Conversation
Concierge reviewer checklist:
For more detailed instructions on completing this checklist, see How do I review a HelpDot PR as a Concierge Team member? |
@dangrous @kadiealexander One of you needs to 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] |
A preview of your ExpensifyHelp changes have been deployed to https://d33ccca1.helpdot.pages.dev ⚡️ |
Hey @kadiealexander and @dangrous — this is coming from #46202. For some reason, the changes I implemented back in Aug/September did not go into production. If something needs to be changes or tweaked in terms of my PR, please let me know and I can get that wrapped up ASAP. I am OoO next week for the holidays, so hoping to have this done before EOW. Thank you! Also, the dev link above, flashes with the information and then it disappears. Not sure what is going on, but I got a video for context: |
Hey @CortneyOfstad I don't think this looks quite right? It looks like maybe the article is split in two, with half on Expensify Classic and half on New Expensify. Also I think the header should be capitalised. ![]() ![]() It also isn't showing up in the Expensify Classic section of the help site, I think maybe we need a separate file in docs/articles/expensify-classic/ to do that but maybe @dangrous can confirm. ![]() |
Yep, you're spot on (see convo here).
|
oh annoyingly I can't suggest changes in places that weren't edited. Well, the top should look like this: title: Manage Travel Member Roles
description: Modify member roles within Expensify Travel
---
-<div id="expensify-classic" markdown="1">
+<div id="new-expensify" markdown="1">
Admins can assign roles to different travel members to determine who they can book travel for (whether for themselves and/or for others) and whether they can adjust administrative settings.
-
-</div>
-
-<div id="new-expensify" markdown="1">
To assign a role to a travel member, And then for the new file in the classic one, just swap out |
Noting that I'm heading OOO for two weeks today, so you may need to grab a US coach to do the final review tomorrow @CortneyOfstad. |
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.
Do we need this additional update? Bolding the Users
? If so we might as well merge this one!
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@CortneyOfstad what's the plan with this PR, are we closing it in favour of the new PR here? |
We should probably either push this one or add the change to the new one, right? seems like a good change to make for consistency |
@dangrous Does it make more sense to just push through the new patch and close this one? The new one has the updated title, but wasn't sure if there is preference in these situations |
So we should definitely push the other one through, but if we want this change (bolding that word) we might as well do this one too. (Rather than recreating the same change on the new PR). |
I'm happy to push this one through, knowing the rest of the changes will be made elsewhere 👍 |
Thanks guys!!! |
🚀 Deployed to staging by https://github.com/dangrous in version: 9.0.83-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.83-5 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.83-5 🚀
|
Explanation of Change
Edits to this page were originally done in August, however, they did not post to production. I've made some slight edits to act as a "refresh" on the page to get a new PR into the world, so it can be launched into production on the help site page. More context here.
Fixed Issues
#46202
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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