-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
feat: linear gradient px
and transition hint syntax support
#48410
base: main
Are you sure you want to change the base?
feat: linear gradient px
and transition hint syntax support
#48410
Conversation
@intergalacticspacehighway Thanks for the PR! I tagged the people that worked on the feature. Thanks for the patience in this slower time, due to holidays and end-of-the-year celebrations. |
const positions = colorStop.positions; | ||
// Color transition hint syntax (red, 20%, blue) | ||
if ( | ||
colorStop.color == null && | ||
Array.isArray(positions) && | ||
positions.length === 1 | ||
) { | ||
const position = positions[0]; | ||
if (typeof position === 'string' && position.endsWith('%')) { | ||
processedColorStops.push({ | ||
color: null, | ||
position: parseFloat(position) / 100, | ||
}); | ||
} else { | ||
// If a position is invalid, return an empty array and do not apply gradient. Same as web. | ||
return []; |
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 think we can change this to share some code better. Do transition hints have to be percentages? Regardless we are doing a lot of the same things, can you try to get rid of the top level if/else here and share some of this logic?
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.
Do transition hints have to be percentages?
They are actually length-percentage
in the spec. But i only added %
support for now in color stops. Linear gradient support mixing %
and px
units which require us to know the dimension, for this we'll have to move color stop parsing to native side. I wanted to avoid that change until the new css parser.
can you try to get rid of the top level if/else here and share some of this logic?
i kinda prefer the explicit checks, also each branch has a tiny comment on top, but i will take another look!
type ParsedGradientValue = { | ||
type: 'linearGradient', | ||
direction: LinearGradientDirection, | ||
colorStops: $ReadOnlyArray<{ | ||
color: ProcessedColorValue, | ||
color: ColorStopColor, |
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 would comment in what scenarios we can have a null color, or otherwise change the typing to make that super clear like
$ReadOnlyArray<{
colorStop | transitionHint
}>
I think they are similar enough to just roll with the comment but it should be clear that that syntax is supported
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.
Added a comment. Thanks!
@@ -1331,7 +1331,13 @@ inline void fromRawValue( | |||
positionIt->second.hasType<Float>()) { | |||
ColorStop colorStop; | |||
colorStop.position = (Float)(positionIt->second); | |||
fromRawValue(context, colorIt->second, colorStop.color); | |||
if (colorIt->second.hasValue()) { |
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.
what is the default value of the color in ColorStop? Will we be able to detect that this is a transition hint vs something like the color black?
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.
here if JS color is null
, that only happens if user passed transition hint. Then the color in ColorStop gets default SharedColor value i.e. HostPlatformColor::UndefinedColor
. So for black color it should be alright!
auto &colorStop = colorStops[i]; | ||
// Skip if not a color hint | ||
if (colorStop.color) { | ||
continue; |
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.
again, curious if a color like black breaks this? Regardless I feel like it would be more future proof and more obvious if you just changed the color in ColorStop to be a std::optional or such
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.
color in ColorStop is SharedColor instance which defaults to HostPlatformColor::UndefinedColor
and it has bool overload to check for undefined color. So i think black or such colors should be fine?
{color: null, position: 0.4}, | ||
{color: processColor('blue'), position: 1}, | ||
]); | ||
}); |
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.
can you add some more? Hints that are invalid (> 100%, non percents, multiple in a row, etc). Some longer ones with more stops and more hints just to stress test things a bit, etc. The code has a lot of branching logic due to the complexity of the syntax we are allowing, so I think we should err on the side of excessive testing to raise our confidence level here
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.
Agree, added.
* linear gradient px support * fix test cases * handle invalid transition hint * fix multiple transition hint * final fixes
@joevilches i added the |
px
and transition hint syntax support
@joevilches has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
linear-gradient(red, 20%, green)
px
support. Combination ofpx
and%
also works.processColorTransitionHint
andgetFixedColorStops
is moved to native code so it can support combination ofpx
and%
units as it requires gradient line length, which is derived from view dimensions and gradient line angle.Changelog:
[GENERAL] [ADDED] - Linear gradient color transition hint syntax and
px
unit support.Test Plan:
Added testcase in processBackgroundImage-test.ts and example in LinearGradientExample.js
Todo
Add testcases for
getFixedColorStops
andprocessColorTransitionHint
in native code for both platforms. That's the only downside of moving it out of JS 🤦