Inconsistent Behavior for CRUD view modals

[ktmud]

Screenshot

Description

In the new React-based Dashboard & Chart CRUD view. The edit modal has animation on both opening and closing, but the edit modal has only opening animation.

Also, I think we should probably consider linking the edit button to opening <dashboard_url?>edit=true in a new window instead of opening the properties modal, because it's much more commonly needed to edit the dashboard layout/charts rather than just the things in properties modal.

Design input

[describe any input/collaboration you'd like from designers, and tag accordingly. For design review, add the label design:review. If this includes a design proposal, include the label design:suggest]

[issue-label-bot[bot]]

Issue-Label Bot is automatically applying the label #bug to this issue, with a confidence of 0.68. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[ktmud]

Cc @nytai

[nytai]

Let's have a conversation here (@junlincc @lilykuang @benceorlai @eschutho @mistercrunch ) about switching over the edit action on the dashboard list view.

Current the edit action only open the dashboard properties modal. Should we just make the call that "Editing a dashboard means opening the dashboard in edit mode and not opening the dashboard properties modal"? It seems that everyone I've talked to advocates for

linking the edit button to opening <dashboard_url?>edit=true in a new window instead of opening the properties modal, because it's much more commonly needed to edit the dashboard layout/charts rather than just the things in properties modal.

I think this should clear up a number of bugs/cosmetic issues around the current dashboard edit experience. Does anyone feel differently?

[ktmud]

+1 for changing the edit button to open dashboard in edit mode.

[mistercrunch]

+1 for changing the edit button to open dashboard in edit mode

Also! In dashboard edit mode, consolidate the 2-3 other modals (filter mapping, colors, auto-refresh) under other tabs under "Edit Dashboard Properties", and make that an Ok dialog (not Save), affect the local redux state, and be kept there until the dashboard's Save is hit.

[junlincc]

+1 for changing the edit button to open dashboard in edit mode

Also! In dashboard edit mode, consolidate the 2-3 other modals (filter mapping, colors, auto-refresh) under other tabs under "Edit Dashboard Properties", and make that an Ok dialog (not Save), affect the local redux state, and be kept there until the dashboard's Save is hit.

Also Edit Chart Properties modal please~ 😉 @nytai

[nytai]

Looks like the portion about the strange saving behavior on the properties modal is covered by https://github.com/apache/incubator-superset/issues/11677

[rusackas]

I want to close this issue since it's such an antique, but dang it... it's still true!

@eric-briscoe / @michael-s-molina / @geido if anyone has bandwidth, it'd be good to look at the modal component, and see if we can nail down the transitions globally, for the sake of the design system.

@jess-dillard / @kasiazjc if anyone has bandwidth, it might be worth (re)considering adding an additional action in the list view to let users edit the dashboard properties OR go right into dashboard edit mode. I'd contend that both have their place.

[kasiazjc]

I want to close this issue since it's such an antique, but dang it... it's still true!

@eric-briscoe / @michael-s-molina / @geido if anyone has bandwidth, it'd be good to look at the modal component, and see if we can nail down the transitions globally, for the sake of the design system.

@jess-dillard / @kasiazjc if anyone has bandwidth, it might be worth (re)considering adding an additional action in the list view to let users edit the dashboard properties OR go right into dashboard edit mode. I'd contend that both have their place.

I think Karan is working on the CRUD views, I'll pass this one to him (not sure what is his github handle). Thanks @rusackas :)

[rusackas]

Was hoping to close this as stale, but alas, it's still inconsistent! Opened a PR that should finally fix this.