-
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
Fix the toggle button text style #40950
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Native40950-android-native.mp4Android: mWeb Chrome40950-mweb-chrome.mp4iOS: Native40950-ios-native.mp4iOS: mWeb Safari40950-mweb-safari.mp4MacOS: Chrome / Safari40950-web-safari.mp4MacOS: Desktop40950-desktop.mp4 |
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.
@hayata-suenaga I think we missed fixing the muted text for Tags
settings. Did we?
|
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.
@hayata-suenaga Few small updates in PR author checklist for your consideration:
- The linked issue does not seem to be correct.
- It would be good to add a test case for
tags
settings too. Something like this:
Steps:
1 Go to theworkspace
settings page
2 Go toTags
page and clickSettings
button
3 Verify that the text next to the toggle button is not muted.
Otherwise LGTM. And tests well too.
@rojiphil thank you for the review 🙇
The linked issue is actually correct, I just didn't link the specific comment that is relevant to this PR 🙇
good idea! I added these to the test steps 😄 |
@hayata-suenaga There are conflicts in the branch |
I think the issue was fixed on the main branch. @rojiphil could you check if the issues have been fixed? 🙇 |
@hayata-suenaga I think the issue is not fixed completely yet. Two observations as seen in the attached test video
40950-inconsistency.mp4 |
I don't believe that is intentional. @shawnborton for double confirmation. @shawnborton, the text next to the button shouldn't be bold like the one below, right? |
Correct, that text should not be bold and should just be our regular weight font at 15px size (normal size). |
There was a change that uses different component for toggle. So after merging made my changes obsolete. so I'm working on achieving the same thing with the changed component |
Does this look okay @shawnborton ? |
@rojiphil we're also updating the UI text from confirming translation -> https://expensify.slack.com/archives/C21FRDWCV/p1714605765597709 |
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.
@hayata-suenaga The changes look good on Categories
settings.
But we missed bringing the earlier changes to Tags
settings as the text is still muted.
Also, I think we should change spend
to expenses
in tags
just like in categories
.
40950-web-safari-001.mp4
Good catch, let's fix that! |
@hayata-suenaga Gentle bump on the fix here |
@shawnborton, sorry for the late response for your question here The distance between the Switch component and subtext is 16px in the current PR as shown in the above screenshot. This is because I'm using the same component and configuration as those used in the Advanced Connection configuration screen, as shown below. The distance between the Switch and the subtexts on the Advanced Configuration screen is 2px? on Figma. But as seen in the above screenshot, it's actually 16px in the production app. Which one should I use, 16px or 2px? Should we use the same distance or should we use different values for these different contexts? If my explanation is unclear, please feel free to ask any questions. 🙇 |
@shawnborton, thank you for your confirmation 😄 I adjusted my PR accordingly. Could you check the screenshots below when you have time? 🙇 Before (16px between the Switch and the subtext)After (4px) |
Looks good to me, thanks! |
@rojiphil, could you do the final check and approve the PR is everything looks good? 😄 |
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 super good to merge. And tests well too. I have updated the checklist with the latest test videos. Thanks.
✋ 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/hayata-suenaga in version: 1.4.74-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.74-6 🚀
|
Details
@shawnborton realized that the text style next to the toggle button is wrong on Categories and Tags settings screens. This PR fixes the style.
This PR also changes the word
spend
in the UI copy toexpenses
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/387834
PROPOSAL: N/A
Tests / QA Steps
Settings
button1 Go to the workspace settings page
2 Go to Tags page and click Settings button
3 Verify that the text next to the toggle button is not muted.
Offline tests
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectionTested on the Chrome browser. Please see the screenshots in the PR description
toggleReport
and notonIconClick
)myBool && <MyComponent />
.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