-
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
Add Light Theme light.js
and clean up colors.js
#22986
Conversation
Screen.Recording.2023-07-17.at.10.11.26.AM.movI've added the light theme V1 in this PR. I think the main tweaks we may make to the theme are in the hovers / icon hovers. We can either QA this with this PR or QA when we go live- do you have the bandwidth to do a first pass of the theme to see if there's anything that needs to be tweaked? I can send screenshots as needed |
Are you able to spin up a test build so I can test it locally? |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Tests pass on adhoc build |
@thesahindia @ 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] |
@thesahindia sorry for the ping, forgot we changed the logic to assign C+ before engineers. I need to have this PR merged today @allroundexperts , let me know if I should have it reviewed internally |
Will finish the review today @grgia! |
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.
Testing well!
Looks good! All set to merge @grgia? |
@allroundexperts thank you again! I appreciate your speedy C+ review here 🙏 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
successHover: colors.greenHover, | ||
successPressed: colors.greenPressed, | ||
transparent: colors.transparent, | ||
midtone: colors.green700, |
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.
hey @grgia, midtone is being using in RequestMoney and Status page, remove it will lead to crash
https://expensify.slack.com/archives/C049HHMV9SM/p1694744871014989
🚀 Deployed to staging by https://github.com/grgia in version: 1.3.71-0 🚀
|
@grgia What do we check in mweb, desktop and iOS? |
@allroundexperts @Li357 Could you help us with what to validate in mweb, desktop and iOS? |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.71-12 🚀
|
Details
In this PR, I am adding a V1 of our light theme as well as doing a first pass clean up of our colors.js file.
color.js
light.js
to house the new light theme.green____
todark____
to match the theme name.greenIcons -> darkIcons
greenborderLighter
,blueTextPreview
colors.js
getLoginPagePromoStyle()
that was referencing old colorsFixed Issues
$ #21016
Tests
To Test V1 of Light Mode:
Add
import lightTheme from './light';
and change the export tolightTheme
indefault.js
We created a test branch with the above and had the build QA'd here - QA Light Theme #23158
This will be QA'd again when we actually release it
Verify that no errors appear in the JS console
To test the color changes (removed color keys) in dark theme:
Offline tests
QA Steps
To test the color changes (removed color keys) in dark theme:
No light theme QA, will be done when theme switching is live.
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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android