Page MenuHomePhabricator

[native] Phase out `ColorPicker` and replace with `ColorSelector`
ClosedPublic

Authored by abosh on Jun 30 2022, 12:26 PM.
Tags
None
Referenced Files
F2830442: D4418.id14197.diff
Fri, Sep 27, 7:38 PM
F2830440: D4418.id14043.diff
Fri, Sep 27, 7:37 PM
Unknown Object (File)
Thu, Sep 26, 5:55 PM
Unknown Object (File)
Thu, Sep 26, 5:55 PM
Unknown Object (File)
Thu, Sep 19, 4:23 AM
Unknown Object (File)
Thu, Sep 19, 4:23 AM
Unknown Object (File)
Thu, Sep 19, 4:23 AM
Unknown Object (File)
Thu, Sep 19, 4:16 AM
Subscribers

Details

Summary

Related Linear issue with full context here.

This diff removes ColorPicker from native (gone but not forgotten) and replaces it with ColorSelector. Along with renaming all instances of ColorPicker this also refactors ColorPickerModal to use ColorSelector as the ColorSelectorModal.

Test Plan

Tested the mobile app to make sure the ColorSelector worked and looked as expected. Devices tested: iPhone SE, iPhone 13 Pro, iPhone 13 Pro Max, iPhone 12 Pro, Pixel 4, Nexus 5.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

abosh edited the test plan for this revision. (Show Details)
tomek requested changes to this revision.Jul 1 2022, 8:26 AM
tomek added inline comments.
native/chat/settings/color-selector-modal.react.js
62–63

In the old code we had two colors: color and threadInfo.color. In the new code we're only using color. Shouldn't we use threadInfo.color? When they can be different?

This revision now requires changes to proceed.Jul 1 2022, 8:26 AM
abosh added inline comments.
native/chat/settings/color-selector-modal.react.js
62–63

I thought the same thing, but I did some digging and color is generated in ThreadSettings where it is initialized to threadInfo.color (as colorEditValue):

image.png (1×1 px, 300 KB)

So they are initialized to the same value. However, throughout ThreadSettings, colorEditValue is updated to match changes made to the state. For example:
image.png (878×1 px, 274 KB)

So colorEditValue and threadInfo.color seem to be the same, but colorEditValue gets updated more...but the only thing that is confusing is in the old ColorPicker the defaultColor prop (color) takes precedence over the oldColor prop (threadInfo.color), which is a fallback if color is undefined:
image.png (524×978 px, 99 KB)

To me, the variable naming is not clear. defaultColor sounds like the color that should be set if the user has never changed the thread color, ever. oldColor sounds like the "current" color that will only become "old" if the user decides to change the color. But then threadInfo.color would also be updated, which is the original value of oldColor.

That's why the new ColorSelector only takes one prop: currentColor. By the reasoning above, I went with color instead of threadInfo.color as currentColor which gets passed into ColorSelector. I tested both threadInfo.color and color as the prop that get passed into ColorSelector, testing new thread creation and all and they both seemed to work the same.

tomek added inline comments.
native/chat/settings/color-selector-modal.react.js
62–63

Sounds reasonable and agree - the old naming was more confusing.

This revision is now accepted and ready to land.Jul 4 2022, 4:03 AM
This revision now requires review to proceed.Jul 5 2022, 7:23 AM

Looks good!

Devices tested: iPhone SE, iPhone 13 Pro, iPhone 13 Pro Max, iPhone 12 Pro, Pixel 4, Nexus 5.

Thanks for being so thorough with the testing here!

(btw seems like it'd be fairly easy to convert ColorSelectorModal to a functional component if you wanted to try doing that)

This revision is now accepted and ready to land.Jul 5 2022, 10:55 AM

(btw seems like it'd be fairly easy to convert ColorSelectorModal to a functional component if you wanted to try doing that)

Yes, this sounds fun!