-
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
Theme switching: Going Live! (Add preferences menu entry for ThemePage) #21669
Theme switching: Going Live! (Add preferences menu entry for ThemePage) #21669
Conversation
@thesahindia Please 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 please ignore you're mention, @grgia is gonna review this |
Adding Hold, since we will not be merging this until we go live |
Commenting for Melvin, still on hold |
@chrispader we'll need to merge main since there's been an update to the translate hook. I temporarily made the changes in this commit so design can QA but I'll remove it before we merge so we can QA all the settings in this PR. Also, what are your thoughts on the drawer animation here? It feels a little jagged with the rerendering, but Im not sure if there's much we can do with push to page here Screen.Recording.2023-12-05.at.3.35.31.PM.mov |
done 👍 |
I don't think there's much we can do here either. Probably due to the rendering of the lottie animation. i can take a look into this by time. |
@chrispader yeah the jaggedness matches changing priority mode, so I think it's fine. Personally, I think the fix is actually not navigating back to preferences on button click for the settings that rerender large parts of the app |
@chrispader for tests, here's some paths to get started: Anonymous User
Brand New User
Logged in User
|
@grgia the current default theme is Should we change this to |
Also, should i add your comment to the test steps or is it just for us? |
@chrispader came here to tell you to swap it to SYSTEM, but youre three steps ahead of me 😄 |
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 like in places where we do themes[CONST.THEME.DEFAULT];
system isnt working
hmmm clicking on the button isnt working Screen.Recording.2023-12-13.at.12.07.40.PM.mov |
We had to uncomment the line in |
Rebuilding |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
PS I cancelled that build ^ |
@chrispader if you merge main it should fix lint |
QA Brand New UserHere we are testing: Set OS theme to Dark Screen.Recording.2023-12-13.at.2.43.15.PM.movLog out, set OS theme to Light split.mov |
not sure if this is related, doesn't seem so. can check tmrw! |
Looked into that kind of error and I don't think it's related either, looks like a react navigation bug |
Reviewer Checklist
Screenshots/VideosAndroid: NativeSee adhoc Android: mWeb ChromeN/A iOS: NativeScreen.Recording.2023-12-13.at.5.41.23.PM.moviOS: mWeb SafariScreen.Recording.2023-12-13.at.5.41.02.PM.movMacOS: Chrome / SafariScreen.Recording.2023-12-13.at.6.32.22.PM.movMacOS: DesktopScreen.Recording.2023-12-13.at.6.40.02.PM.mov |
waiting on the test build for android bc my local android still will not build 🙃🤖 |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
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.
🕺 🎊 LGTM 🎊 🕺
✋ 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/grgia in version: 1.4.13-0 🚀
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.4.13-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.13-8 🚀
|
@grgia
Details
Fixed Issues
$ #21670
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
Not needed.
QA Steps
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
See #21666
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android