search

shipt / segmented-arc-for-react-native

Segmented arc component for React Native
MIT License
93 stars 10 forks source link

Remove .isRequired from emptyColor prop #101

Closed crosenfrisk closed 2 months ago

crosenfrisk commented 2 months ago

The other day while testing Stats I saw a warning on the Ratings detail screen related to emptyColor on SegmentedArc. To address, I would like to propose removing .isRequired from the propTypes since emptyColor is not being used or required in feedback-summary-component.js in Neutron -- there the circle on Ratings detail screen will always show 100% (no emptyColor portion for the circle).

When I mocked the removal on Neutron -- the removal of .isRequired on propTypes did resolve the issue of the warning for emptyColor on segments.

If there are other suggestions for how to resolve this issue on Neutron rather than on this repository, please let me know and I can redirect my attention there.

_*Here I am also proposing adding a default definition for emptyColor as #F3F3F4 to match grey_100 on Neutron._ removed default color suggestion after conversation with Kyle and Tanner's comment, made a great point that this repository is being used by more than just Neutron. Point to consider in the future is a default color (such as transparent, per Tanner).

crosenfrisk commented 2 months ago

Also, I saw the ask in CONTRIBUTING.md after I made this branch's name. Let me know if you need me to close this PR and open a new one to follow the convention:

Branch prefixes we use: feature/: for features support/: for general refactoring hotfix/: something broke and we need to fix it now

crosenfrisk commented 2 months ago

Love it

Thank you!!!! I appreciate your guidance in understanding what to address, Jahon for pointing me to you in the first place, and Kathryn for Zooming with me this morning to help me understand why when I made changes in Neutron it wasn't "working" but then addressing in SegmentedArc was like 🎉 So, thanks all!

TannerDrayton commented 2 months ago

While this works for the shopper app, is it too presumptuous of us to assume that the default color should be what we've set it to? I wonder if transparent could be an option 🤔

crosenfrisk commented 2 months ago

Jahon and I are communicating in a DM. I shouldn't have removed .isRequired from emptyColor in this public repository. I am working on reverting to its original condition and will address the warning in Neutron instead.