rancher / dashboard

The Rancher UI
https://rancher.com
Apache License 2.0
442 stars 243 forks source link

Merge all the modals in a single component and functionalities #8207

Open cnotv opened 1 year ago

cnotv commented 1 year ago

Description

Create a single modal component with slots to extend current functionalities.

As outcome we want:

Notes

A second round research to identify all of the cases is still required! Example of cases which may still cause issues:

Context

This issues presents different challenges.

Style inconsistencies between the modals

Examples:

The standard we're initially looking at with buttons in the bottom right:

GenericPrompt Screenshot 2023-02-20 at 11 57 44

We have these other patterns, to give context about what variances we have and what we need to move from:

AddonConfigConfirmationDialog Screenshot 2023-02-20 at 11 58 06

PromptRemove Screenshot 2023-02-20 at 11 58 22

Screenshot 2023-05-04 at 10 08 50

Existence of several modals components

Code-wise we have several components without a common base and sometimes in other paths: ./shell/components

Screenshot 2023-02-20 at 12 11 01

Any path but found with this.$modal

Screenshot 2023-04-27 at 17 53 39

./shell/dialog

Screenshot 2023-02-20 at 12 11 34

Two different patterns adopted

We currently request modals in 2 different flavors: as action, with the same modal and as markup, with custom versions.

Action

In some cases the modal are requested with an action as in GenericPrompt, e.g.:

      this.$store.dispatch('management/promptModal', { // sometimes is 'cluster/promptModal', not sure why
        component:      'GenericPrompt',
        componentProps: { ... }
      });

PROS

Markup

In other cases the modals/prompts are placed directly in the markup several time 🤔 (there's more than these 3 in the screenshot)

Screenshot 2023-04-27 at 18 01 57

PROS

gaktive commented 1 year ago

Whatever we design here needs to be documented in Storybook so we ensure we have a consistent modal experience.

cnotv commented 1 year ago

As outcome of the conversation:

Basic requirement for the issue is:

cnotv commented 1 year ago

Another point which needs to be addressed, some modals allow HTML and some others not. By default for i18n we often use markup in it.

cnotv commented 1 year ago

@gaktive at the current state we keep creating new modal styles, e.g. inactive modal, so I'm adding more take aways.

cnotv commented 8 months ago

Design will be reviewed as part of the code refactoring process since we do not have a defined one and is complex for the designer to understand the current state.

gaktive commented 8 months ago

One of the conditions of moving to Vue 3 is tied to requiring a new modal component -- https://github.com/rancher/dashboard/issues/9537

However, we should wait to work on that once we standardize what the modals should look like so we have an easier port. We should discuss this soon so we can get off Vue 2.

cnotv commented 8 months ago

@gaktive this issues is on triage since long, so we omit the redesign and we'll stick to just ask how to align the buttons and that's it.

A full redesign discussion can be done later after we'll publish it in the style-guide.

cnotv commented 8 months ago

@richard-cox since we don't want to get stuck with this issue and we rather want to find a solution, the final goal will also be to don't actually change these 50+ use cases of this.$modal(componentName)

gaktive commented 7 months ago

There's a risk here of breaking extensions that consume these modals, so care needs to be taken since Epinio, Rancher Desktop, Opni, Kubewarden, etc. depend on these.

gaktive commented 7 months ago

Key target: use GenericPrompt as the standard pattern to use (i.e, buttons in the bottom right).

gaktive commented 7 months ago

As part of this work, we need to look at all the modals since it sounds like by digging into this, you almost start.

@cnotv & @richard-cox to provide input.

cnotv commented 7 months ago

Added notes from the remarks of @richard-cox. As I mentioned, it's impossible to foresee the outputted markup and styling by just reading the code, so it would require to actually implement an initial modal.

If we want to simplify in steps, we may starts by creating an actual component without have an actual use, then create separated migrations in chunks, although this will have to turn into an epic.

richard-cox commented 7 months ago

@cnotv there were some wires crossed there.

cnotv commented 7 months ago
  • Given the above it feels like we need to understand how all modals are used, then understand the impact of the required changes (it may be further reaching and harder to do than just fixing the usages of this.$modal)

Technical implementation of the modal has been already described in the issue under Two different patterns adopted and used in the code as displayed in the lists Existence of several modals components.

richard-cox commented 6 months ago

Two different patterns adopted covers the same underlying modal. GenericPrompt referenced by actions is just a component that's shown inside of the PromptModal referenced in markup.

Would you be able to confirm

My question's regarding the scope of this ticket rather than how a new modal might look. Is it to either...?
- Create a new root modal (containing the single this.$modal) and convert all the above cases to use it
- Convert the custom locations to use the generic components
- Another approach
cnotv commented 6 months ago

The idea is to have a common layout, which can be used later, a single component, which allows the 2 different approaches to be requested and a way to override the default layout. There should be a common ground for all of them and provide all the possible solutions: callback, async, etc.

The technical details are obviously something which is ongoing during development. There will be a Storybook for the component and layout, plus unit tests to verify the functionalities.