appsmith
appsmith copied to clipboard
appsmithorg
[Bug]: Add JS for icon property in buttons
Is there an existing issue for this?
- [X] I have searched the existing issues
Current Behavior
EX: Starting with the button widget, We see that the button color cannot be dynamically changed and the one in the table widget can. This is an Inconsistency and similar situations can be see across these properties.
We propose we look at this from the other way around and make all properties JS convertible, and then check what are the failing scenarios.
- [ ] What changes do we need to this from the platform POV.
Steps To Reproduce
- D&D button widget
- try setting the button color dynamically
- see that you do not really have access to JS
Environment
Production
Version
Cloud
@somangshu this is becoming a headache now.
I have assigned a similar issue to @vibhandikyash #11387 where the idea is to convert all fields to JS first in a dp and see which fields don't need JS and remove JS option for those.
Can we discuss about this issue in slack/sync once and move ahead?
@dilippitchika surely we can do that, Our idea is to find a change to enable it everywhere rather than making the changes individually. Hence I needed an input from @riodeuno. Feel free to start a thread around this.
@dilippitchika @somangshu @vivekverma2312
Here is how we can go about it:
- Enable JS for all properties which don't have it. Except for the ones which are nested or have panels (For example; Table columns (panel) and Table level styling vs Column level styling (nesting))
- When enabling, please take into account any side-effects or any potential breaking changes to existing applications.
- All new JS convertible properties will now need to have a validation.
- Finally, as always, we need someone from the QA team to be involved in the process from the start to finish. This will ensure that we are testing these changes while communicating the caveats properly.
The first point talks about an exception because, there is not fixed sequence to evaluations, unless one property depends on another. I can discuss this in detail when we come it.
Thanks @riodeuno
@dilippitchika we will need to a way to list and track all scenarios. We can work on this together if you have a template in mind, We will keep @vivekverma2312 informed about all the discussion.
The only thing is that this looks like high effort from the PRD / Dev / QA POV. We might have to push this back to later. Let me know if you agree / disagree
Missed this for some reason, I will create a template and track this forward.
We have fixed a lot of these in the property pane project. Adding ones that got missed out
We need the icon property to have JS in button, button group and menu buttons.
Greetings @sanjus-robotic-studio thanks for showing interest 🎉 , This is all yours. Assigning this to you now.
Please don't forget to read the Contribution Guidelines. Would appreciate if you can open a PR within the next 2 days. let us know here
Thanks for this @sanjus-robotic-studio, @Tooluloope can you please review this pr and run it through our workflows?
CC @somangshu
Checked in Button, Button group and Menu button, JS option for Icon property is present only for Button widget. It is not present in Button group and Menu button.
@sanjus-robotic-studio can you also add it in menu button and button groups?
But in the previous Pr itself I have added it. Can you check and let me know
You are right, i can see it there, but not in our environment. Let me ask someone to check, @Tooluloope @yaldram can one of you check?
@sanjus-robotic-studio Could you help fix this?
CC @dilippitchika
https://user-images.githubusercontent.com/31691737/199193670-ee7ff157-f456-48ce-b566-674196b67a0a.mov
@Tooluloope can you check this ? https://github.com/appsmithorg/appsmith/pull/18031