Improve the modal UX for the snapshot module

[vedhav]

Problem:

It takes a small amount of time to close the modal and reopen it causing a bad UX, it would be nice if we could just overlay the secondary modal popups over the primary one creating a more intuitive UX for the user.

For testing I'm using the same app code mentioned in the PR #929

Changes:

1. Introduction of custom_modal_dialog() to create a custom modal dialog that can be placed along with other modal dialogs without issue, allowing these "custom modal dialogs" to be easily shown and hidden using JS.
2. Fix the namespace issue because of duplicate HTML ids for snapshot_name causing the snapshot_name in the file upload dialog box to not have any value (now we have ids new_snapshot_name and new_file_snapshot_name).

Please have a look at the GIF to see the new change. (~7MB might take a while to load)

[chlebowa]

Thanks for this, I'll have a look.

As a general comment, could you please not upload gifs? Their constant animation is distracting, especially if it's not as fluid as this one. Screencasts are much better, you play them when you want.

[m7pr]

What about gif+screencast?

[chlebowa]

I'm not sure I like invisible inputs. But then that is what conditinalPanel does and we have a lot of those in the modules.

What do you think @insightsengineering/nest-core-dev?

[vedhav]

I'm not sure what you mean by "invisible inputs" but if you're referring to the bottom right Computing ... which is the only conditionalPanel UI I can see over here then I totally agree and I'm not a fan of them. I would love to have component-specific loading with something like the waiter package if I had to choose.

[chlebowa]

I mean input widgets that have display: none.
And by conditinalPanel I mean shiny::conditionalPanel in several module UI functions.

[kartikeyakirar]

Should we make this as widget in teal.widget ?

[vedhav]

Should we make this as widget in teal.widget ?

If you're proposing just the custom_modal_dialog then it makes perfect sense to me. But nothing more than that because in my head teal.widgets help teal module developers create UI that is very common to be reused in many modules. And the only reusable component I see from here would be custom_modal_dialog, We'd have to come up with a better name.
But I stand against moving the snapshots module into teal.widgets.

[kartikeyakirar]

The initial design had the table and snapshot manager positioned side by side, while the new design places it below the table. This might be a UX design question. When the snapshot manager is located side by side, it remains visible even when there is a long list of filters. Should we consider keeping it in this layout?

With long list of filters

[chlebowa]

While I like having them side by side, we shouldn't get too attached to that layout because in apps with many modules the mapping matrix will push the snapshots down anyway. See here.

[chlebowa]

I'm not sure I like invisible inputs. But then that is what conditinalPanel does and we have a lot of those in the modules.

What do you think @insightsengineering/nest-core-dev?

I would really like to hear some opinions on this.

[kartikeyakirar]

with many modules the mapping matrix will push the snapshots down anyway

Ah, I see. In that case, I suppose this is bound to happen anyway. From a user's perspective, having a visible snapshot manager could make it more accessible. should we consider adding tabs in modal ?

[vedhav]

I'm not sure I like invisible inputs. But then that is what conditinalPanel does and we have a lot of those in the modules.

I don't like when the hidden elements persist to stay in the DOM forever. But, I'm not bothered by the hidden elements if they are removed when they are not needed which is the case with this. The nested popup is part of the main popup and will be completely removed once the main popup is removed.

[vedhav]

should we consider adding tabs in modal ?

How about moving it to the top? and having a scroll to the table as suggested here

[kartikeyakirar]

I'm not sure I like invisible inputs. But then that is what conditinalPanel does and we have a lot of those in the modules.

Invisible inputs may have their advantages in specific scenarios, but personally, share both yours and vedha opinion on this. This is primarily due to the added complexity they introduce when managing visibility. Even when hidden, these inputs persist in the DOM, consuming resources and potentially causing latency in your application.

[kartikeyakirar]

@vedhav moving the snapshot manager table won't resolve this because a similar edge case exists there too. Users can encounter the same problem when adding multiple snapshots and pushing the filter table down.

[chlebowa]

This discussion is branching into two. Let's keep the layout part in the relevant issue, shall we?

[chlebowa]

While I like the UX improvement, I feel DX is severely deteriorated. Modals are created in the ui function and their handling (showing, hiding, updating) is done in the server. It could be just me because I like having everything in one place but it does feel disjointed.

Also, there is no showModal and removeModal so it still seems to me that the modals are not created and destroyed in response to buttons but rather persist in an hidden state. They may be removed when the parent modal is closed but that is only because the snapshot manager placed in a modal (which is created by a different module), what happens if we decide to take it out at some point?

[vedhav]

what happens if we decide to take it out at some point?

You're right, Removing the modal is an effect of being inside the modal which is the intended use of this "nested" modal. It's supposed to be nested inside a modal.

Modals are created in the ui function and their handling (showing, hiding, updating) is done in the server

I would disagree that the modals are created in the UI. Shiny still creates them on the server side, so if you're disconnected from the server even the showModal/hideModal does not work, only the easyClose by clicking outside or pressing escape works because they are implemented on the JS side.

[chlebowa]

I would disagree that the modals are created in the UI.

OK, defined.

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  97      88  9.28%    9-71, 93-106, 109-116, 131-146
R/get_rcode_utils.R                  32       1  96.88%   52
R/include_css_js.R                   24       0  100.00%
R/init.R                             80      31  61.25%   114-121, 145, 164-185, 215-217, 219-220
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_filter_manager.R           107      36  66.36%   49-55, 62-70, 79-84, 228, 233-246
R/module_nested_tabs.R              149      58  61.07%   64-137, 153, 205, 227, 253
R/module_snapshot_manager.R         236     180  23.73%   87-140, 168-169, 173, 178-191, 193-198, 205-206, 210-212, 214-220, 223-233, 236-252, 258, 264-279, 293-316, 319-330, 333-339, 353, 374-397
R/module_tabs_with_filters.R         73      33  54.79%   60-95, 127, 140
R/module_teal_with_splash.R          94       4  95.74%   67, 88, 153-154
R/module_teal.R                     141      32  77.30%   68, 71, 158-159, 165, 196, 204-205, 227-259
R/modules_debugging.R                19      19  0.00%    25-45
R/modules.R                         155      26  83.23%   119, 132, 224-227, 241-246, 257-261, 391-434
R/reporter_previewer_module.R        18       2  88.89%   26, 30
R/show_rcode_modal.R                 20      20  0.00%    16-37
R/tdata.R                            53       1  98.11%   153
R/teal_data_module-eval_code.R       27       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                  6       0  100.00%
R/teal_reporter.R                    60       5  91.67%   65, 116-117, 120, 137
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      59      12  79.66%   135-148
R/utils.R                           111      27  75.68%   112-139
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   109-371
R/zzz.R                              11       7  36.36%   3-14
TOTAL                              1723     644  62.62%

Diff against main

Filename                       Stmts    Miss  Cover
---------------------------  -------  ------  -------
R/module_snapshot_manager.R      +27     +23  -1.15%
TOTAL                            +27     +23  -0.76%

Results for commit: 218169b

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

    1 files    19 suites   10s ⏱️
202 tests 200 ✔️ 2 💤 0 ❌
402 runs  399 ✔️ 3 💤 0 ❌

Results for commit 218169b.

♻️ This comment has been updated with latest results.