-
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
[NO QA] Update Insights.md #56418
base: main
Are you sure you want to change the base?
[NO QA] Update Insights.md #56418
Conversation
First pass at the update (will follow with ChatGPT improvements)
ChatGPTs version
A preview of your ExpensifyHelp changes have been deployed to https://44bb220b.helpdot.pages.dev ⚡️ |
Concierge reviewer checklist:
For more detailed instructions on completing this checklist, see How do I review a HelpDot PR as a Concierge Team member? |
@ikevin127 @flaviadefaria 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] |
NB: I need a screenshot to be updated on the article. Specifically, we need to have the |
Ok! I have replaced the outdated image with the new image, and have made sure it follows the exact file name convention and path so it replaces the outdated image completely. |
@johncschuster - sorry ive been OOO and now a bit sick. will get to this review tomorrow <3 |
@johncschuster - looks good! approved these changes. i think internal eng still needs to merge tho! |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as duplicate.
This comment was marked as duplicate.
@ikevin127 @ @CortneyOfstad 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] |
Sorry for all the additional assignees, everyone! I tried reapplying PullerBear a few times, and while it did find assignees, I needed an internal engineer for this one. @dangrous can you be my huckleberry? |
haha sure - will take a look shortly! |
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.
Some formatting notes. Image swap seems good too, though you won't see it until it's live since it uses the live link.
3. Use the filter options to select the categories, tags, employees, or any other parameter | ||
4. Make sure that View in the top right corner is set to the pie chart icon | ||
5. You can view any dataset in more detail by clicking in the **View Raw Data** column | ||
--- |
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.
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.
Oh, interesting! I'm not sure why the original author included those in this article. I don't feel strongly one way or another about those, personally.
When I review the article, I wonder if those are supposed to indicate a break in the different actions listed in the article. Like, "Export Your Insights Data" is a standalone action, separate from "Create a Custom Export Report for Expenses". I wonder if those lines are meant to distinguish that.
Personally, I don't feel blocked on this. What are your thoughts, @maddylewis?
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.
i would:
- change the wording from "Your Insights Data" - that is weird
- the formatting overall is totally fine!
|
||
{% include faq-begin.md %} | ||
# FAQ |
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.
# FAQ |
if we put back the {% include faq-begin.md %}
line, it will automatically title it FAQ and use the dropdown, so we can remove this line
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.
I think the outcome of this comment will be dependent on what we find about the collapsible FAQ section. I'll wait on that.
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.
Note - this matches the hierarchy we have on all other docs pages, BUT technically for accessibility it really shouldn't be an h1.... but we can leave it here to match. At some point we really need to work on accessibility across the app though.
|
||
Need something specific? Contact your Account Manager for assistance! | ||
|
||
--- |
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.
--- |
Don't need this one at the end if we use the FAQ tag
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.
I think the outcome of this comment will be dependent on what we find about the collapsible FAQ section. I'll wait on that.
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.
I think even without the FAQ block we don't need this line
Co-authored-by: Daniel Gale-Rosen <[email protected]>
Co-authored-by: Daniel Gale-Rosen <[email protected]>
Co-authored-by: Daniel Gale-Rosen <[email protected]>
Co-authored-by: Daniel Gale-Rosen <[email protected]>
okay cool! Didn't know we were dropping the FAQ thing. Let me take one more look at this with that in mind, i think we might want to update the headers for the FAQ in that case |
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 assuming we want to keep (all of) the lines. I'd at least remove the last one, since it's unnecessary, but everything else is just up to your preference.
|
||
Need something specific? Contact your Account Manager for assistance! | ||
|
||
--- |
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.
I think even without the FAQ block we don't need this line
|
||
{% include faq-begin.md %} | ||
# FAQ |
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.
Note - this matches the hierarchy we have on all other docs pages, BUT technically for accessibility it really shouldn't be an h1.... but we can leave it here to match. At some point we really need to work on accessibility across the app though.
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/465266
PROPOSAL:
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
)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