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

Reduce number of (almost identical) duplicate dialog components #830

Closed
Steve-Mcl opened this issue Jun 27, 2022 · 11 comments · Fixed by #900
Closed

Reduce number of (almost identical) duplicate dialog components #830

Steve-Mcl opened this issue Jun 27, 2022 · 11 comments · Fixed by #900
Assignees
Labels
feature-request New feature or request that needs to be turned into Epic/Story details
Milestone

Comments

@Steve-Mcl
Copy link
Contributor

Description

While working in and around the flowforge frontend, it was noticeable the number of almost identical components for confirmation type tasks.

It would be advantageous for the majority of simple "OK/Cancel" "Please enter a name", "Are you sure?" dialogs to be replaced with a simple utility - something like const result = await app.showMessageBox(options); if(result === MESSAGEBOX_OK) {...}

This then negates the need for separating the "dialog instantiation code" in the page from the "action code" in the component

Additionally, also very useful for something as simple as notifying success or failure of new features (until a more async method is developed)

@Steve-Mcl Steve-Mcl added feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do labels Jun 27, 2022
@sammachin sammachin transferred this issue from FlowFuse/flowfuse Jun 27, 2022
@knolleary
Copy link
Member

Additionally, also very useful for something as simple as notifying success or failure of new features (until a more async method is developed)

That is coming via #515

To keep this focussed on one thing - the requirement is to reduce the boilerplate we have for simple confirmation type dialogs.

@Steve-Mcl
Copy link
Contributor Author

Hi @joepavitt

As requested in our meeting, here are 2 approaches that would help (from a developers POV) to save time, reduce code splitting/state management complexities, reduce lots of duplication and would help (from a designers POV) with consistent appearance across all pages.

NOTE: The pseudo code below is less relevant than the approach which is to reduce the burden on the programmer of managing hundreds of boilerplate vue components and disjointed code.

Simple yes/no/cancel type dialogs...

const result1 = await ff.showMessageBox("Are you Sure", ff.MESSAGEBOXBUTTON.OKCANCEL, "alert")
if (result1 === ff.MESSAGEBOXRESULT.OK) {
  //do stuff
}

const result2 = await ff.showMessageBox("Are you Sure", { 
  buttons: [
    ff.MESSAGEBOXBUTTON.OK, 
    ff.MESSAGEBOXBUTTON.CANCEL, 
    {label: "Maybe", id: "maybe", cb: "onMaybe"} 
  ] 
})
if (result2 === ff.MESSAGEBOXRESULT.OK) {
  //do stuff
}

Simple dialogs with input capability

const myForm = {
      id: 'my-form', //where its pinned in the dom
      fields: {
        email: EmailField({
          label: 'Email',
          validations: [
             ff.validator({ validator: required, text: 'This field is required' }),
             ff.validators.email
          ],
        }),
        password: PasswordField({
          label: 'Password',
          validations: [
             ff.validator({ validator: required, text: 'This field is required' }),
             ff.validators.password
          ],
        }),
        rememberMe: CheckboxField({
          label: 'Remember Me',
        }),
      }
}

//simple sync version 
const formData = await ff.form.show(myForm);

//callback version
ff.form.show(myForm, (data) => {
  //do stuff with data
});

// on submit version
function onSubmit(data) {
  //do stuff with data
}

Vue stuff...

<ff-form>
   id="my-form"
   :form="form"
   @submitted="onSubmit"
   @error="onError"
 />
 <button
  class="btn"
  submit="true"
  :form="form?.id"
 >
  Sign In
 </button>
<ff-dialog>

Notes

the API and methods are all up for debate - what I hope we can achieve is to avoid the disjointed and distributed code (i.e. state management, event emitters, if/else event handlers, form show code in the index.vue, business logic in the dialog.vue, boilerplate templates etc etc. )

@joepavitt I realise you asked for this issue comment to be centered around AdminStackDeleteDialog.vue however as I am less familiar with vue3 and best practices than you, I didn't want to lead you in the design with a worse solution.
However, I do hope from the pseudo examples provided you can see the direction I would hope we can achieve whereby the current situation is greatly simplified and code duplication is reduced.

@joepavitt
Copy link
Contributor

@Steve-Mcl

Okay, so I have a few thoughts/comments, but to play devil's advocate, I will counter with the following, which is how things (could) operate, if make a simple change of moving the show/hide logic into the ff-dialog, and have simple show() or close() functions on that component to call.

Simple yes/no/cancel type dialogs...

<ff-dialog ref="my-dialog" message="Are you Sure?">
   <template v-slot:actions>
      <ff-btn kind="secondary" @click="doStuff()">Cancel</ff-btn>
      <ff-btn kind="secondary" @click="doStuff()">Maybe</ff-btn>
      <ff-btn kind="primary" @click="doStuff()">OK</ff-btn>
   </template>
</ff-dialog>
this.$refs['my-dialog'].show()

Simple dialogs with input capability

<ff-dialog ref="my-dialog" @close="onClose" @submit="onSubmit">
   <!-- If we just use the standard okay/cancel buttons, then only need one template here: -->
   <template>
      <ff-form>
         <ff-text-input v-model="form.input1" type="email" :validator="emailValidator" required />
         <ff-text-input v-model="form.password" type="password" :validator="passwordValidator" required />
         <ff-checkbox v-model="form.checkbox" label="My Checkbox"></ff-checkbox>
      </ff-form>
   </template>
</ff-dialog>
methods: {
   onClose () {
      this.$refs['my-dialog'].close()
   },
   onSubmit () {
      console.log(this.form)
   }
}

Also would require implementing the validator which we don't support at the moment (but need anyway) in forge-ui-components.

With your proposal, we would be required to build out an entire set of Form components that are driven by that API (and ensure they're documented well somewhere). The effort involved here, for what I see as the only benefit being that this is written in JS, instead of HTML, I am not sure I can justify?

The reason I was particularly interested in AdminStackDeleteDialog.vue is because it had some interesting "edge" cases, i.e. rendering of particular items depending on state, that I was interested to see how a JS API could handle. In the pattern I've set here, where content is defined in HTML, this is really simple.

It is worth noting though, for the case of a simple confirm/cancel Dialog, which would be a common use case across a lot of the FlowForge platform, I can certainly see (and would 100% support) why we would want to introduce simplicity in the form of something like:

const dialog = await Dialog.show("Are you sure?")
if (dialog.cancelled) {
   // was cancelled
} else if (dialog.confirmed) {
  // was confirmed
}

Where this HTML/component was defined in a single place as we currently having with ff-notification-toast in layouts/Platform.vue. The opening/closing of this dialog would then be possible via a similar architecture to what we have in place with services/alerts.js.

In both situations, knowledge is required of the available components/API, so both require a learning curve per-say for new developers. I do also understand the advantage, and how nice to read it is, to have all "logic" contained in the JS, i.e. if a user has hit the "Delete Project" button, the logic for the confirmatory dialog is contained within the onDelete function.

My primary hesitation rests on the effort required to build out another set of components that support the agreed-upon API, and then where this content is documented, and how flexible that API would need to be to support existing use cases we have (let alone future ones we haven't considered).

@joepavitt
Copy link
Contributor

My proposal (for now) would be that we build out a service to enable the very simple confirm/cancel use case with a single message. I would also be open to building a similar component/service for delete dialogs, as they always follow a pattern of "type out this thing's name" before the delete button is enabled.

I think those two things alone would sweep through a big chunk of our dialogs anyway. For the dialogs with forms, for now, I would like to keep with driven by HTML, although agree there is logic that should be stored in ff-dialog (mostly the open/close state mgmt) that needs to be moved.

@joepavitt
Copy link
Contributor

joepavitt commented Jul 5, 2022

I am certainly not opposed to moving to a full object/API-driven approach in the future, but can't quite justify (at least) a week's worth of work to do that given the list of other stuff we have on the todo list in the current position we are in.

@knolleary
Copy link
Member

I think a common approach to simple confirm dialogs would be beneficial.

On the Delete case - I don't propose we use that exact pattern (type name to confirm) everywhere we need to confirm a delete - that would soon get annoying for users. We have it on the Project as its the critical resource of the platform.

For the more generic form dialog, I agree that should be considered a future/maybe feature. A number of our more complex dialogs (create stack etc) would actually benefit from being refactored as dedicate create-XYZ pages (like we do for Projects and Templates). We should aim to reduce our use of dialogs to the alerts/confirmations covered by the simple case.

@joepavitt
Copy link
Contributor

@Steve-Mcl I am satisfied with the recent changes in forge-ui-components to resolve that side of this issue. As such, future changes will be contained within the /flowforge repo and so I'll be transferring this issue over there.

@joepavitt joepavitt transferred this issue from FlowFuse/forge-ui-components Jul 29, 2022
@sammachin
Copy link
Contributor

@joepavitt just to clarify my understanding here:
We now have components in the forge-ui-components pacakge for these dialogs.
But there is now work to swap out the code in the forge app to use them?
But new confirmations can be built with these components today?

So the outstanding task that needs to be prioritised is the retrofit of the components to existing code?

@joepavitt
Copy link
Contributor

@sammachin not quite. We've been using the ff-dialog for some time from forge-ui-components. However, @Steve-Mcl proposed some good improvements to the simplicity of use of these dialogs. Whilst we aren't doing all of these straight away (Steve would like a fully implemented JS API to control and design Dialogs), we are working towards slimming them down with the following steps:

  • Move the open/close state management into the ff-dialog (now completed in forge-ui-components, and flowforge has just had recent changed merged in flowforge with Remove all manual show/close logic for ff-dialog (now contained within the component) #829)
  • The creation of a JS API to show a simple confirm/delete Dialog within FlowForge, which would not expand more than an injection of text into the box and a "confirm" action being defined. (still todo)

In the future, we will consider the use cases put forward by Steve above for a wider covering API, but personally I have hesitations on that at the moment due to the diversity of use cases and layouts we currently use Dialogs for.

@joepavitt joepavitt added this to the 0.9 milestone Aug 19, 2022
@Steve-Mcl
Copy link
Contributor Author

@joepavitt

The creation of a JS API to show a simple confirm/delete Dialog within FlowForge, which would not expand more than an injection of text into the box and a "confirm" action being defined. (still todo)

Is this captured in an issue or task anywhere?

@Steve-Mcl
Copy link
Contributor Author

For the sake of 0.9 verification, I have tested the dialogs replaced by this task & all ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request that needs to be turned into Epic/Story details
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants