Skip to content
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

[Feature]-[800]:Deprecate form button widget #3613

Closed
hetunandu opened this issue Mar 18, 2021 · 21 comments · Fixed by #14510
Closed

[Feature]-[800]:Deprecate form button widget #3613

hetunandu opened this issue Mar 18, 2021 · 21 comments · Fixed by #14510
Assignees
Labels
Enhancement New feature or request Form Widget Verified When issue is retested post its fixed Widgets & Accelerators Pod Issues related to widgets & Accelerators Widgets Product This label groups issues related to widgets

Comments

@hetunandu
Copy link
Member

Description

Since we have resetWidget as a trigger function, we can deprecate the form button widget added with every form widget. This needs to be a backwards compatible change.

@github-actions github-actions bot added Widgets Product This label groups issues related to widgets UI Building Pod and removed Actions Pod labels Mar 18, 2021
@riodeuno
Copy link
Contributor

riodeuno commented Apr 1, 2021

  • Update Form widget configs to use normal button widget
  • Migrate DSLs to convert all "FORM_BUTTON_WIDGET" to be "BUTTON_WIDGET"
  • Migrate all "FORM_BUTTON_WIDGETS" which have the reset action to use the resetWidget action instead
  • Jest test for migrations
  • Documentation update if needed.

@dilippitchika
Copy link
Contributor

https://www.notion.so/appsmith/Form-Btn-to-Btn-36f43ca84d8046e1b0b62de587f81cc7 @somangshu @riodeuno can you review the PRD once?

This is written with the assumption that we migrate all buttons to be form buttons by default, and only allow form control edits when the button is in a form. Will need design changes if we decide to proceed.

@riodeuno
Copy link
Contributor

@dilippitchika I cannot access the notion doc. Also, here is an updated approach which I had commented in the PR by @techbhavin #5917 (comment)

@momcilo-appsmith
Copy link

Hey @dilippitchika I don't think you guys need any design help here, I think this is pretty straightforward.

@dilippitchika
Copy link
Contributor

dilippitchika commented Jan 27, 2022

Adding the PRD Here

Objective

Instead of having 2 different buttons one traditional and one for form we only need one button throughout Appsmith. This helps manage the apps well both for users and us.

As part of this, we need to evaluate how to support the traditional button in forms as well and work in all current button cases. This will involve thorough migration check.

Success Metrics *

Goal Metric
Use a single button component in forms and non-form use cases No of bugs raised on form button inconsistency should decrease. No of buttons broken for people’s existing apps should be 0.
Requirement User story Comments
Button when dragged on the canvas should support form specific configuration. Form specific config, Disable on invalid form data, Reset form on Success As a user, I want to use the button from the widget panel inside my form to perform exactly same actions as form buttons do so that I can add new actions in my forms. This may need a separate form section in our buttons which can be dynamically disabled
Button when not a part of the form widget, should disable the form specific configuration. There should be a clear demarcation about the configs being form specific configs. As a user, when I use the button in other places I don’t want to worry about form configuration
None of the existing functionality either in forms or in the regular buttons should not break

Documentation

  • Changes to the button widget documentation explaining the form configurations in buttons with cases on when it’s enabled and how to configure it

Design change

Screenshot 2022-01-27 at 11 16 34 PM

Introduce the reset form on success and disable on invalid forms option in the button widget for now. Update the tooltips to the following
Disable for Invalid Forms - Works only inside Form widget. Disables the button if any required widget in form is invalid.

Reset Form on Success - Works only inside Form widget. Resets the widgets in the form if the On Click event succeeds.

Dev Note

  • ButtonWidget will be removed
  • FormButtonWidget will be renamed to ButtonWidget
  • The widget type will be BUTTON_WIDGET
  • We migrate all FORM_BUTTON_WIDGET references to BUTTON_WIDGET.
  • The resetFormOnClick will continue to work as it does today, however, it will now also show in the property panes of button widgets which does not exist directly inside a form.
  • The following text should be changed.

Original: Resets the fields within the parent form when the click action succeeds"
New: Resets the fields of the form, on click, if this widget exists directly within a Form widget.

  • The following text should be changed.

Original: Disables the button when the parent form has a required widget that is not filled
New: Disabled if the form is invalid, if this widget exists directly within a Form widget.

These two fields needs to be organised properly, my suggestion would be add another section called "Form options" and put the above two property controls in this section.

@dilippitchika
Copy link
Contributor

@momcilo-appsmith please review the above info once, this will be an addition to the content pane for button.

@somangshu
Copy link
Contributor

@dilippitchika added a dev small dev note from the last research, please see if it is not inline

@dilippitchika
Copy link
Contributor

@somangshu it is inline, let's currently name it as "Form options" as you suggested.

@dilippitchika
Copy link
Contributor

@somangshu assigning to us to clear this up and proceed

@somangshu
Copy link
Contributor

somangshu commented Mar 31, 2022

We have two ways to move forward

  • Migrate all references, Create a checklist to test, Create all relevant tests, Use all the form references in internal apps to test all the use cases: Confidence 80%
  • Do not touch the older references, Do not add any migration, Create v2 to use button, show error on all old formButtons through validations or the above suggestion. Confidence 100%

@ghost
Copy link

ghost commented Apr 5, 2022

A user would like to use a buttons isDisabled property, which doesn't currently exist in the Form Button Widget.

@somangshu
Copy link
Contributor

@AS-Laguna Once we complete the above activity the form button will get replaced by a normal button and hence the isDisabled property will be available for the dev.

@somangshu
Copy link
Contributor

somangshu commented May 26, 2022

@dilippitchika we now have #12291 merged and available soon, We can go ahead and pick this up with the second strategy we had discussed

  • Do not touch the older references, Do not add any migration, Create v2 to use button, show error on all old formButtons through validations or the above suggestion. Confidence 100%

Can you also think of any other dependency that we need to address before picking this up?

@somangshu somangshu removed the Needs Design needs design or changes to design label May 26, 2022
@dilippitchika
Copy link
Contributor

dilippitchika commented May 26, 2022

There's one issue which we may need to fix when we are taking this up.

Link to video - https://www.loom.com/share/56cc7ae651e34f8b88db0d730805e0b3

Today our "reset form on success" field when switched on also resets if there's an error in the API which is incorrect, i believe this could be fixed here. @somangshu

@techbhavin please take a look

@dilippitchika
Copy link
Contributor

Stats

Stat Values
Reach 1200
Effort (months) 0.75

@Nikhil-Nandagopal Nikhil-Nandagopal changed the title Deprecate form button widget [Feature]-[800]: May 30, 2022
@Nikhil-Nandagopal Nikhil-Nandagopal changed the title Deprecate form button widget [Feature]-[800]: May 30, 2022
@dilippitchika dilippitchika changed the title [Feature]-[800]: [Feature]-[800]: Deprecate form button widget May 30, 2022
@dilippitchika
Copy link
Contributor

@techbhavin assigning this to you, please discuss with somangshu and me once before picking this up

@somangshu
Copy link
Contributor

Summury of older discussion

  • Two ways we could move ahead in > Check this out
  • Finally POA

@somangshu
Copy link
Contributor

somangshu commented Jun 7, 2022

Actionable

@techbhavin
Copy link
Contributor

@somangshu @dilippitchika

Here, FormButton Widget have Property Disabled Invalid Forms and Reset From on Success this property is not available in the button widget

  1. In the Button widget Property Panel above two Properties will be visible always or these two properties are visible only buttons inside a Form widget ?

@dilippitchika
Copy link
Contributor

@techbhavin we disable the fields when it's not in a form, please check the PRD above

@close-label close-label bot added the QA Needs QA attention label Jul 22, 2022
@Nikhil-Nandagopal Nikhil-Nandagopal changed the title [Feature]-[800]: Deprecate form button widget [Feature]-[800]:Deprecate form button widget Jul 22, 2022
@chandannkumar chandannkumar added Verified When issue is retested post its fixed and removed QA Needs QA attention labels Jul 29, 2022
@Nikhil-Nandagopal Nikhil-Nandagopal added the Widgets & Accelerators Pod Issues related to widgets & Accelerators label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Form Widget Verified When issue is retested post its fixed Widgets & Accelerators Pod Issues related to widgets & Accelerators Widgets Product This label groups issues related to widgets
Projects
None yet