-
-
Notifications
You must be signed in to change notification settings - Fork 21.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
[RTL] Add support for image dynamic updating, padding, tooltips and size in percent. #80410
Conversation
37ed04f
to
1901b84
Compare
1901b84
to
924f1ec
Compare
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.
Functionality-wise looks ok.
I wonder if there is a simpler way to make update_image()
than having to call a method with 7 unused arguments (in case you want to change 1 thing); it's likely the most efficient/intuitive way. The other solution I could think of is replacing arguments with Variant array that has to be filled depending on the mask, but that would be more error-prone.
Another concern is parameter creep in add_image()
🙃 It can't be solved without named arguments though.
There is some changes to be rebased @bruvzg |
924f1ec
to
47a0858
Compare
<param index="6" name="key" type="Variant" default="null" /> | ||
<param index="7" name="pad" type="bool" default="false" /> | ||
<param index="8" name="tooltip" type="String" default="""" /> | ||
<param index="9" name="size_in_percent" type="bool" default="false" /> |
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 full list of parameters is starting to be quite convoluted, that will be an awkward API to use IMO.
Struct support would be great here, passing a configuration struct with selected optional members defined would probably be better.
47a0858
to
135ab10
Compare
39538f6
to
be42c54
Compare
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.
Needs rebase, otherwise seems good to me.
be42c54
to
bc6585a
Compare
Thanks! |
RTL only part of #69751