Add a color to the options in the picker when using Windows #13174
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
|
||
| // Windows will reuse the text color of the select for each one of the options | ||
| // so we might need to color accordingly so it doesn't blend with the background. | ||
| if (getOperatingSystem() === CONST.OS.WINDOWS) { |
There was a problem hiding this comment.
NAB I think that other platforms were unaffected by the dark theme change and override these styles, so checking for Windows might be unnecessary. That being said, it doesn't hurt to do so.
There was a problem hiding this comment.
Yeah, that is what I thought and I was going to leave it without it but I didn't want to cause a regression.
There was a problem hiding this comment.
Makes sense. Let's keep it this way
There was a problem hiding this comment.
Having the OS check here is the same as having a platform-specific check and our README shares pretty strong opinions about adding them:
Accordingly, don't even bother writing a platform-specific code block because you're just going to need to undo it.
Can you please take the time to remove this? My guess is the same as Carlos's and that having the color there isn't going to affect the other platforms, but let's be sure to test for regressions.
| this.items = _.map(this.items, item => ( | ||
| { | ||
| ...item, | ||
| color: '#002140', |
There was a problem hiding this comment.
We should add this as a CONST
There was a problem hiding this comment.
Ideally, it should be in the themes but Shawn said we don't have this one yet. DO you want me to add it as constant no matter what?
There was a problem hiding this comment.
It seems like we have a colors switch PR coming up, based on this comment. I'll ping Georgia to clean this up once we deprecate this other colors.
There was a problem hiding this comment.
I would love it if this was moved to a constant or maybe to our style.variables. Anytime we leave code like this, even temporarily, that's opening chances for contributors to see it and copy it, assuming it's a good pattern to follow.
From what I've heard, we are not moving quickly on that theme-switcher, and even when they do, it will be hard to find this and fix it, so it could live here for a long time if someone forgets about it.
|
|
Co-authored-by: Carlos Martins <cmartins@expensify.com>
melvin-bot
bot
requested review from
madmax330 and
rushatgabhane
and removed request for
a team
|
@rushatgabhane @madmax330 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] |
Reviewer Checklist
Screenshots/VideosWeb** Mac OS** ** Windows ** Mobile Web - ChromeMobile Web - Safari
Desktop
iOSAndroid
|
| if (getOperatingSystem() === CONST.OS.WINDOWS) { | ||
| this.placeholder = _.isEmpty(this.placeholder) ? {} : { | ||
| ...this.placeholder, | ||
| color: '#002140', |
There was a problem hiding this comment.
For future reference, in case someone stumbles on this PR, we didn't create a constant for this because we'll have to clean it up anyways when the light theme is up soon.
|
Checklists are complete! I'll merge this to fix the blocker! |
Add a color to the options in the picker when using Windows (cherry picked from commit df97c3c)
…-13174 🍒 Cherry pick PR #13174 to staging 🍒
|
🚀 Cherry-picked to staging by @luacmartins in version: 1.2.33-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by @luacmartins in version: 1.2.33-7 🚀
|
|
🚀 Deployed to production by @luacmartins in version: 1.2.33-7 🚀
|
| } | ||
|
|
||
| componentDidMount() { | ||
| this.setDefaultValue(); | ||
|
|
||
| // Windows will reuse the text color of the select for each one of the options |
There was a problem hiding this comment.
It's probably not very ideal to have this logic here and I think the constructor is a better place for it. The reason is, componentDidMount() isn't run until after it's rendered, but we want those values to be there before things are rendered.
|
|
||
| // Windows will reuse the text color of the select for each one of the options | ||
| // so we might need to color accordingly so it doesn't blend with the background. | ||
| if (getOperatingSystem() === CONST.OS.WINDOWS) { |
There was a problem hiding this comment.
Having the OS check here is the same as having a platform-specific check and our README shares pretty strong opinions about adding them:
Accordingly, don't even bother writing a platform-specific code block because you're just going to need to undo it.
Can you please take the time to remove this? My guess is the same as Carlos's and that having the color there isn't going to affect the other platforms, but let's be sure to test for regressions.
| this.items = _.map(this.items, item => ( | ||
| { | ||
| ...item, | ||
| color: '#002140', |
There was a problem hiding this comment.
I would love it if this was moved to a constant or maybe to our style.variables. Anytime we leave code like this, even temporarily, that's opening chances for contributors to see it and copy it, assuming it's a good pattern to follow.
From what I've heard, we are not moving quickly on that theme-switcher, and even when they do, it will be hard to find this and fix it, so it could live here for a long time if someone forgets about it.
|
Sure, I can make all those changes, sorry about that! |
|
I also brought up in #expensify-open-source the discussion about not supporting Windows since I find it interesting taking into account that is the most used OS. |
|
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
Details
Changing the colors of the items was really tricky since the component does not offer that possibility, I ended up fixing it by doing this: lawnstarter/react-native-picker-select#440
Fixed Issues
$ #13127
PROPOSAL: N/A
Tests
On a Windows OS:
On a Mac OS:
Offline tests
QA Steps
On a Windows OS:
On a Mac OS:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots/Videos
Web - Mac
Web - Windows
Mobile Web - Chrome
Web - Safari
Desktop
iOS
Android
