-
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
[HelpDot] New structure to support platforms #27851
Conversation
.github/scripts/createDocsRoutes.js
Outdated
process.exit(1); | ||
return 1; |
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.
Revert. return 1 is not the program exit code. It still exists with code 0.
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.
nice catch. is this a javascript thing?
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.
Yeah I think so
title: New Expensify | ||
--- | ||
|
||
{% include platform.html %} |
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.
_includes/
does not have platform.html
file
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 done to make it easier for review.
It'll be added in next PRs!
Deployment of help site is paused, so it won't break anything
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 breaks local testing though 😅 I hope the new PR lands soon
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.
yes, let's get this one merged asap
In new expensify there is no section |
@s77rt updated the step to mention it's applicable for expensify-classic only |
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.
Since:
- there's no way to test this locally (until @rushatgabhane created follow-up PR)
- HelpDot deploys are disabled
- The code seems fine
I don't think it makes sense to delay here. Going to merge this.
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.73-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.73-1 🚀
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.74-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.74-3 🚀
|
cc: @dangrous @roryabraham @arosiclair
Details
This is a massive PR, but most files are dummy
Coming Soon
files.For the review you'll want to focus on the following files:
docs/_data/_routes.yml
- Contains 2 platforms and 12 hubs inside each platform. This will be used bycreateDocsRoutes
to generate the routes.yml file used for defining title, description, icon path etc for navigation and the UI..github/scripts/createDocsRoutes.js
- Adds all the articles and sections defined indocs/articles
toroutes.yml
docs/expensify-classic/hubs/
- Contains 12 hubs for expensify classicdocs/new-expensify/hubs/
- Contains 12 hubs for new expensifyFixed Issues
$ #27150
PROPOSAL: N.A.
Tests
npm run createDocsRoutes
docs/_data/routes.yml
.Verify that the structure generated resembles this doc
bank-accounts-and-credit-cards
has articles inside it.bank-accounts-and-credit-cards
for expensify-classic has a section, and articles inside it.Verify that removing/adding hubs throws an error on running
createDocsRoutes
Remove from _routes.yml
i. Remove a hub from
docs/_data/_routes.yml
eg: integrationsii. Verify that
npm run createDocsRoutes
throws an error.Delete from
docs/new-expensify/
i. Delete
docs/new-expensify/account-settings.html
ii. Verify that
npm run createDocsRoutes
throws an error.Offline tests
QA Steps
Verify that https://help.expensify.com/#gsc.tab=0 is still working.
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
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)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
N.A.