fix: Update Expensify Help version badges to look like badges#64567
fix: Update Expensify Help version badges to look like badges#64567srikarparsi merged 9 commits intoExpensify:mainfrom
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? |
|
@rojiphil @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] |
|
@dannymcclain I updated. |
|
@daledah do you have updated screenshots/videos too? |
docs/_sass/_main.scss
Outdated
| line-height: 16px; | ||
| } | ||
|
|
||
| #platform-tabs > .badge{ |
There was a problem hiding this comment.
Can we just make this a more generic class? Like even just a simple .badge with no specificity before it.
There was a problem hiding this comment.
Also, why aren't we setting a font-size on these badges? Please see Danny's guidance.
There was a problem hiding this comment.
We already have the font size here:
Line 1115 in bc12ce0
There was a problem hiding this comment.
Hmm yes but let's not make the CSS so specific. Please DRY this up into a clean .badge class, where we'll need to include a font size. We don't want super specific selectors in our CSS files. Thanks!
|
What's the latest on this one? |
Review feedback has to be worked upon. |
|
@rojiphil I updated. |
|
CSS feels better to me, thanks for hearing me out! Let us know when we have updated screenshots/videos. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppNA Android: mWeb Chrome64567-mweb-chrome-002.mp4iOS: HybridAppNA iOS: mWeb Safari64567-mweb-safari-002.mp4MacOS: Chrome / Safari64567-web-chrome-002.mp4MacOS: DesktopNA |
@shawnborton Here are the test videos MacOS: Chrome / Safari64567-web-chrome-002.mp4iOS: mWeb Safari64567-mweb-safari-002.mp4Android: mWeb Chrome64567-mweb-chrome-002.mp4 |
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @daledah for the update.
@srikarparsi Code changes LGTM. Approving for your review.
Agree! The badges also feel a little wide to me, can we double-check that the horizontal padding is |
Height is 36px |
|
@rojiphil can you try to match the specs outlined in this comment? The height should be |
Ah! Sorry. I didn't notice that. |
|
@shawnborton @rojiphil Is this good?
Or some more spaces like this one:
|
|
That does look better, again we just need to confirm a height of 28px and font size of 11px. |
|
@rojiphil I updated. |
|
Thanks @daledah for making the changes. @shawnborton @dannymcclain Here are the updated test videos. LGTM. Please confirm if this is fine. MacOS: Chrome / Safari64567-web-chrome-003.mp4Android: mWeb Chrome64567-mweb-chrome-003.mp4iOS: mWeb Safari64567-mweb-safari-003.mp4 |
|
Nice, I think that's feeling good to me. |
|
Same! |
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @shawnborton @dannymcclain for the confirmation.
@srikarparsi Changes LGTM. Approving again for your review.
docs/assets/js/main.js
Outdated
|
|
||
| function selectNewExpensify(newExpensifyTab, newExpensifyContent, expensifyClassicTab, expensifyClassicContent) { | ||
| newExpensifyTab.classList.add('active'); | ||
| newExpensifyTab.classList.add(!expensifyClassicTab && !expensifyClassicContent ? 'badge' : 'active'); |
There was a problem hiding this comment.
What are these changes needed for?
There was a problem hiding this comment.
If the page can be switched via buttons between New Expensify and Classic, we do not want to change the button style.
These changes are to ensure that we display badge only when either New Expensify or Expensify Classic page is displayed. Otherwise, if the page content is applicable for both, use active. But I am wondering now if there is a use case where the page content is applicable for both as I could not find one. @srikarparsi Can you please point out to one such usage? Thanks.
There was a problem hiding this comment.
Yeah I'm actually not sure which is why I asked. @maddylewis do you know the answer to this question? Basically, are there any pages which are applicable for both New Expensify and Expensify Classic?
There was a problem hiding this comment.
there shouldn't be, no! i was thinking pricing, but even the breadcrumb paths are slightly different in these two resources:
- https://help.expensify.com/articles/expensify-classic/expensify-billing/Billing-Overview
- https://help.expensify.com/articles/new-expensify/billing-and-subscriptions/Billing-Overview
that said, each article is under the expensify-classic or new-expensify path. so even if content was applicable to both, there would be two separate articles (in today's setup at least).
There was a problem hiding this comment.
sounds good thank you! Then in that case, we should be able to remove this extra code and just use the badge class right @rojiphil?
There was a problem hiding this comment.
Yeah. Agree.
@daledah We only need badge class here. Let us remove the extra code related to active. Thanks.
| tab.innerHTML = 'Expensify Classic'; | ||
| tab.id = 'platform-tab-expensify-classic'; | ||
| tab.classList.add('active'); | ||
| if (!newExpensifyContent) { |
There was a problem hiding this comment.
Same for these changes. And why not write it the same as the ternary above?
srikarparsi
left a comment
There was a problem hiding this comment.
Just have two questions before merging
|
Hey @daledah, any update on this one? |
|
@srikarparsi Sorry for the delay 🙏, I updated. |
|
Can you please merge main? It looks like a test is failing |
|
checks passed now after re-running |
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.1.79-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.1.79-11 🚀
|




Explanation of Change
Fixed Issues
$ #64093
PROPOSAL: #64093 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/AAndroid: mWeb Chrome
iOS: Native
N/AiOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
N/A