mui / toolpad

Toolpad: Full stack components and low-code builder for dashboards and internal apps.
https://mui.com/toolpad/
MIT License
1.27k stars 282 forks source link

Confusing CTAs in the queryState editor #322

Closed prakhargupta1 closed 2 years ago

prakhargupta1 commented 2 years ago

Duplicates

Latest version

Current behavior 😯

Confusing CTAs in the queryState data binding dialog box.

Screenshot 2022-04-27 at 12 03 32 AM

Expected behavior 🤔

It should be Cancel and Update.

Steps to reproduce 🕹

Steps: Try editing a queryState

Context 🔦

No response

Your environment 🌎

No response

Janpot commented 2 years ago

Currently the CTAs are in line with the behavior. the input is immediate, i.e. the changes are immediately reflected in the application. Mentioning this because the solution entails more than merely updating the button texts, the behavior needs to be updated as well.

oliviertassinari commented 2 years ago

This makes me think of a generic UX problem: When should we use the autosave pattern (A) vs. manual save (B)? I guess:

Janpot commented 2 years ago

In the case of code: B because code is in a broken state half of the time while editing

prakhargupta1 commented 2 years ago

Closing this issue. New query dialog box has: Cancel, Remove, Save which avoids confusion.

bytasv commented 2 years ago

when it's code: B because it's how VS Code behaves by default?

That's not true:

By default, VS Code requires an explicit action to save your changes to disk, Ctrl+S

@prakhargupta1 I think either this issue or new issue could serve to resume discussion about auto-save vs manual save - we still have places where we auto-save (component editor) and others where we have to manually save.

I think no matter which approach is used it should be consistent through the app. Having auto-save everywhere might not be always possible? 🤔 I think explicit save with a good UI indicator would work better and could be used everywhere without creating a confusion or unexpected behaviour

Janpot commented 2 years ago

I think no matter which approach is used it should be consistent through the app.

It is consistent. The application auto-saves every atomic change that doesn't result in a broken application. The goal is to protect the user's work as much as possible. Atomic changes that result in a broken application (e.g. code in an editor is invalid most of the time while typing) have to be committed manually. I believe we need to differentiate:

  1. auto-saving the application state after a change (e.g. drag component in the view, update property value)
  2. committing a set of staged changes as a single atomic update. (e.g. code updates to a binding, code component). auto-saving each keystroke here only results in storing tons of broken application states.
bytasv commented 2 years ago

Oh right, thanks for clarifying. I guess what gets me confused some times is that changing a name of query/connection gets saved automatically, while all other props require explicit "save" click - that part IMO would be worth unifying and putting under save, same as other fields behave in the dialog 🤔

Janpot commented 2 years ago

Yes, we should revise those name change flows.

bytasv commented 2 years ago

I'd like to revise a bit how we avoid duplicating names. We should probably not make any naming choices for the user. And we should probably allow more duplicates (e.g. you should be able to have two pages with a component that is named the same)

If I'm not mistaken I read that we are using names for data binding? Wondering if we could use Id's instead so we can allow even duplicate names to be used if users prefer

Janpot commented 2 years ago

Wondering if we could use Id's instead so we can allow even duplicate names to be used if users prefer

That would make the logic in bindings hard to follow. In Pro code we use descriptive variable names to increase readability, we should let users do the same in Toolpad.

bytasv commented 2 years ago

Ahhh, ok, now I understood... 🤦 yeah, it makes total sense to keep it that way, ignore my comment...

prakhargupta1 commented 2 years ago

Based on the above discussion, I'll create a new issue for name change, auto-save, manual-save occurrences. cc @gerdadesign