-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Range input #6739 #7052
Range input #6739 #7052
Conversation
@@ -64,7 +64,7 @@ const trackColorStyle = (props) => { | |||
else { | |||
defaultTrackColor = getRGBA( | |||
normalizeColor(props.theme.rangeInput.track.color, props.theme), | |||
props.theme.rangeInput.track.opacity || 1, | |||
props.theme.rangeInput.track.opacity || undefined, |
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.
If we remove the fallback to 1
here will this be backwards compatible?
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.
I belive this should be fine becuase the original implementation was shortcircuting the normalizedAlpha inside getRGBA function to pass alpha channel value as 1 always and it was simply not honouring the 8-hex value with alpha value in the first place. with the updated code being changed to undefined allows the alpha channel value to be passed appropriately and regular 6-hex value works as well. Of course, this is my understanding. we can further think through it again just to ensure the updated code is still backward compatible.
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.
Thank you for clarifying, I see that a fallback opacity is built into getRGBA
function that falls back to 1, so it seems reasonable to just have:
getRGBA(normalizeColor(props.theme.rangeInput.track.color, props.theme), props.theme.rangeInput.track.opacity)
I don't think we need the || undefined
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.
The snapshot test is now added and the redundant undefined short circut is also removed.
6e5a9f1
to
6a222de
Compare
@sulaymon-tajudeen-hpe Can you also add a snapshot test that captures the 8-digit HEX use case ? |
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.
Thank you for the fix!
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.
There are some places in the StyledRangeInput.js
file where we still have props.theme.rangeInput.track.opacity || 1
or opacity || 1
(see lines 82 and 120). Just wanted to understand why we are keeping it on those lines
Both lines 82 and 120 did not seems to make any difference with || 1 on the 8-digit hex code, that is why I did not remove them initially. For the purpose of cnsistency and since || 1 is already taken care of from the original getRGBA function that falls back to 1, the || 1 is now removed from both lines 82 and 120. |
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.
I'm not seeing the opacity coming through on cases where we specify an 8 digit hex within the RangeInput theme. For example if we specify something like this in the theme only the thumb color is respecting the opacity
rangeInput: {
thumb: {
color: '#7f03fc20',
},
track: {
lower: {
color: '#fc03a925',
},
upper: {
color: '#705a6923',
},
},
}
Closing due to inactivity, feel free to reopen |
What does this PR do?
fix opacity not honoured for 8-hex+alpha for range input component track color.
Where should the reviewer start?
What testing has been done on this PR?
How should this be manually tested?
Do Jest tests follow these best practices?
screen
is used for querying.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #6739
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Background compatible