Skip to content

Conversation

@MatiPl01
Copy link
Member

@MatiPl01 MatiPl01 commented Oct 17, 2025

Summary

This PR changes the format of the SVG brush value passed from JS to CPP in CSS animations from the single value to the object with colorType and value props. This will align better with SVG's implementation of the brush type.

@MatiPl01 MatiPl01 self-assigned this Oct 17, 2025
@MatiPl01 MatiPl01 force-pushed the @matipl01/svg-brush-css-value branch from 34183be to ef2ae7c Compare October 17, 2025 14:12
@MatiPl01 MatiPl01 force-pushed the @matipl01/css-colors-object-representation branch from 36afb32 to 6495352 Compare October 17, 2025 14:26
@MatiPl01 MatiPl01 requested a review from piaskowyk October 17, 2025 14:30
@MatiPl01 MatiPl01 changed the title chore: Change color representation from number or string to object chore: Change CSS color representation from number or string to object Oct 17, 2025
}

return {
return () => ({
Copy link
Member

Choose a reason for hiding this comment

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

Why it needs to be an function instead of an object? I don't see a clear reason why 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I have some processors like this one:

export const processMarginHorizontal: ValueProcessor<DimensionValue> = (
  value
) => {
  const result = parseDimensionValue(value);

  if (!result) {
    return;
  }

  return () => ({
    marginLeft: result,
    marginRight: result,
  });
};

They split a single property into multiple ones as I expect only marginLeft, marginRight, marginTop and marginBottom in CPP and don't want to pass others like marginHorizontal, etc. This reduces unnecessary complexity but makes it impossible to return and object from the value processor as it will be interpreted as separate properties. To handle this case, I decided to return a function for processors which result should be considered as separate properties instead of a single object.

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit tricky, but I see why you used this approach. The most readable solution would be to add an additional property that describes the type of data, but this would introduce extra overhead. What do you think would be more beneficial for us in the long term - the current solution with lambdas or adding an additional property type to the object?

Comment on lines 15 to 16
SVGBrush::SVGBrush(const folly::dynamic &dynamicValue)
: CSSColorBase<SVGBrushType, SVGBrush>(SVGBrushType::Transparent) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a small nitpick: since you already have that constructor in CSSBaseColor:

template <ColorTypeEnum TColorType, typename TDerived>
CSSColorBase<TColorType, TDerived>::CSSColorBase()
    : channels{0, 0, 0, 0}, colorType(TColorType::Transparent) {}

I think here you could just call:

SVGBrush::SVGBrush(const folly::dynamic &dynamicValue)
    : CSSColorBase<SVGBrushType, SVGBrush>() {

bool SVGBrush::canConstruct(const folly::dynamic &value) {
return value.isNumber() || value.empty() ||
isValidColorString(value.asString());
colorType = type;
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why we only assign the color type when it's different from RGBA. This could potentially be problematic in the future then someone will add logic based on color type because the default color type value here is Transparent, but the actual value is RGBA.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to introduce unnecessary complexity with variants so I decided that all colors have 4 RGBA channels and the color type. If the type is different from the RGBA type, the color channels can be ignored.

Currently supported remaining color types for SVGs are Transparent and CurrentColor. Both of them cannot be represented as RGBA values, since the Transparent color depends on another one in the interpolation pair (the color we interpolate from/to) and the CurrentColor is treated as a keyword (discrete value) as I can't obtain the current color easily in reanimated (at least I don't know how) and it is managed by SVG.

Copy link
Member Author

Choose a reason for hiding this comment

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

What else do you suggest? Should I make the color channels value optional or introduce a variant for the value and either store 4 RGBA channels or a keyword/sth else indicating that the color is Transparent/CurrentColor?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if i miss understand, but I thought that we can just do it like that:

SVGBrush::SVGBrush(const folly::dynamic &dynamicValue)
    : CSSColorBase<SVGBrushType, SVGBrush>(SVGBrushType::Transparent) { 1Code has comments. Press enter to view.
  const auto [type, value] = parseDynamicValue(dynamicValue);
   colorType = type; // <-- just always assign the type, no mater if it is a SVGBrushType::Rgba / Transparent etc 
  if (type == SVGBrushType::Rgba) {
    *this = SVGBrush(value.asInt());
    return;
  }

I don't want to introduce variants too - this is not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Sorry for misunderstanding. Yes, we can do this but it is not necessary as the SVGBrush constructor for numbers already assigns the SVGBrushType::Rgba type, so we will end up with assigning the same value twice. Both solutions are correct, though.

@MatiPl01 MatiPl01 marked this pull request as draft October 21, 2025 11:21
@MatiPl01 MatiPl01 changed the title chore: Change CSS color representation from number or string to object chore: Change SVGBrush representation from number or string to object Oct 21, 2025
@MatiPl01 MatiPl01 force-pushed the @matipl01/svg-brush-css-value branch from ef2ae7c to 4ed7ecc Compare October 22, 2025 07:17
Base automatically changed from @matipl01/svg-brush-css-value to main October 22, 2025 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants