Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove .isRequired from emptyColor prop #101

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Remove .isRequired from emptyColor prop #101

merged 3 commits into from
Jul 30, 2024

Conversation

crosenfrisk
Copy link
Contributor

@crosenfrisk crosenfrisk commented Jul 30, 2024 •
edited
Loading

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
Copy link
Contributor Author

crosenfrisk commented Jul 30, 2024 •
edited
Loading

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

TannerDrayton
TannerDrayton reviewed Jul 30, 2024
View reviewed changes
src/SegmentedArc.js Outdated Show resolved Hide resolved
rkyle35242
rkyle35242 approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@rkyle35242 rkyle35242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it

@crosenfrisk
Copy link
Contributor Author

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
Copy link

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 🤔

rkyle35242
rkyle35242 requested changes Jul 30, 2024
View reviewed changes
Copy link
Member

@rkyle35242 rkyle35242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emptyColor doesn't need to be a new prop

rkyle35242
rkyle35242 reviewed Jul 30, 2024
View reviewed changes
src/SegmentedArc.js Outdated Show resolved Hide resolved
rkyle35242
rkyle35242 approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@rkyle35242 rkyle35242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we're talkin'

@crosenfrisk crosenfrisk changed the title Remove .isRequired from emptyColor prop and add default value to emptyColor Remove .isRequired from emptyColor prop Jul 30, 2024
@crosenfrisk crosenfrisk merged commit f62cddb into development Jul 30, 2024
3 checks passed
@crosenfrisk crosenfrisk deleted the Claire-Rosenfrisk/CU-86b1fecet/Failed-prop-type-undefined-in-SegmentedArc branch July 30, 2024 16:48
jkhusanov
jkhusanov requested changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@jkhusanov jkhusanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few thoughts...

@crosenfrisk crosenfrisk restored the Claire-Rosenfrisk/CU-86b1fecet/Failed-prop-type-undefined-in-SegmentedArc branch July 30, 2024 16:54
@crosenfrisk
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkhusanov jkhusanov jkhusanov requested changes

@TannerDrayton TannerDrayton TannerDrayton approved these changes

@rkyle35242 rkyle35242 rkyle35242 approved these changes

@calebmackdavenport calebmackdavenport Awaiting requested review from calebmackdavenport calebmackdavenport is a code owner

@chaceburnette chaceburnette Awaiting requested review from chaceburnette chaceburnette is a code owner

@joshuakg joshuakg Awaiting requested review from joshuakg joshuakg is a code owner

@rykugur rykugur Awaiting requested review from rykugur rykugur is a code owner

@kathrynnlester kathrynnlester Awaiting requested review from kathrynnlester kathrynnlester is a code owner

@taylorcore taylorcore Awaiting requested review from taylorcore taylorcore is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crosenfrisk @TannerDrayton @rkyle35242 @jkhusanov