-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Introduce Path SVG property #8648
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
base: main
Are you sure you want to change the base?
Conversation
MatiPl01
left a comment
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.
Left some minor comments for now. Good job on this task overall!
| template class CSSValueVariant<SVGPath>; | ||
| template class CSSValueVariant<SVGPath, CSSKeyword>; |
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 want to heave just a single one here. template class CSSValueVariant<SVGPath> should be sufficient, since the string d property value, as far as I know, must always be a valid path (not something like 'none', etc.).
The CSSKeyword is reserved for values such as 'none', 'auto', etc., which are sometimes allowed together with numeric values, for example see this one:
{"width", value<CSSLength, CSSKeyword>("auto", {RelativeTo::Parent, "width"})CSSLength - allows numbers or number string with % unit (e.g. 10, 25%, etc.)
CSSKeyword - for the 'auto' string
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.
Ok, deleted that
| template class SimpleValueInterpolator<SVGPath>; | ||
| template class SimpleValueInterpolator<SVGPath, CSSKeyword>; |
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.
| template class SimpleValueInterpolator<SVGPath>; | |
| template class SimpleValueInterpolator<SVGPath, CSSKeyword>; | |
| template class SimpleValueInterpolator<SVGPath>; |
same
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.
deleted
| template <typename T> | ||
| explicit SVGPath(T &&value); |
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.
| template <typename T> | |
| explicit SVGPath(T &&value); |
I think that template params aren't needed, at least not in this case, when we know all possible types (the path is always a string, at least for now in the initial implementation).
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.
This isn't a true template type; it only accepts a string&& or a const string&. This specific signature allows the caller to either copy or move a string into the structure. This technique is often used as a shortcut in such case (perfect forwarding)
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.
Ok, fine. I can see this for the first time but I haven't seen much C++ code in other projects before, so I don't know if this is a common practice 😄. I still find this a little bit confusing as there is no constraint on the T value type, so it is not that obvious what is the main purpose of this template constructor.
Can we maybe stick to the simpler approach used in other structs?
| template <typename T> | ||
| SVGPath::SVGPath(T &&value) : command(std::forward<T>(value)) {} |
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.
| template <typename T> | |
| SVGPath::SVGPath(T &&value) : command(std::forward<T>(value)) {} |
This shouldn't be needed as well.
Summary
Adds Path SVG property