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

[Bug] Make single dynamic dropdown clearable #915

Merged
merged 7 commits into from
Dec 4, 2024

Conversation

petar-qb
Copy link
Contributor

@petar-qb petar-qb commented Dec 3, 2024

Description

This PR fixes the case when the value from dynamic Dropdown multi=false is cleared and page is refreshed. This bug was known and it was planned to solve this bug by introducing the Vizro universal placeholder component -> https://github.com/McK-Internal/vizro-internal/issues/1307

This problem is described as the first highlighted problem at the bottom of the https://github.com/McK-Internal/vizro-internal/issues/1356 description.

How it works?
The dmc.DateRangePicker component from the vm.Dropdown._build_dynamic_placeholder is removed and the dcc.Dropdown is used as a dynamic placeholder. This is possible thanks to the new way of calculating new options:: new_options=current_value + new_available_options. This change ensures that the the UI dropdown value is always in a sync with the persisted value, so we don't need to use the dmc.DateRangePicker as the persisted value cannot be changed during the page_build or on_page_load.

See the more detailed description in the Miro board.

More about the dmc.DateRangePicker saga find under the description of this Miro item.

Screenshot

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-12-04 10:37:57 UTC
Commit: 49fbd1b

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so pleased to see this fixed and the weird date picker hack gone 🎉 Thank you for all your perseverance getting it fixed and all the careful writeup or it so we don't forget what we did. It's been quite a saga indeed!

@petar-qb petar-qb requested a review from antonymilne December 4, 2024 06:58
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 🥳 I didn't review the dynamic filter PR so I didn't have much context, but your detailed PR description and issues gave me all the necessary information to understand this PR 💯

I have a question unrelated to this PR, so feel free to merge this PR first and then answer :) I noticed in the dev example that there's a dcc.Loading component on the dynamic filters. Where is this coming from? I can't find it in the dropdown, so it must be in the action code.

I saw your comment about possibly changing the visual design, which I agree with. I'm also wondering if the dynamic controls need any loading visual at all. For components, it makes sense due to longer loading times, but the controls update quickly, right? If you look at BI tools, their dynamic controls update silently without any loading visuals.

@petar-qb
Copy link
Contributor Author

petar-qb commented Dec 4, 2024

Hi @huong-li-nguyen, and thanks for taking the time to review this PR!

I noticed in the dev example that there's a dcc.Loading component on the dynamic filters. Where is this coming from?

If you look at BI tools, their dynamic controls update silently without any loading visuals.

The dcc.Loading component originates from the new vm.Filter.build implementation, which is also responsible for rendering the dynamic filter loading UI. I agree there’s room for refinement here, and I’d be glad to improve the UI for these controls.
One potential improvement could be disabling the controls temporarily during the loading process, rather than switching to a full loading state. This would still indicate that a process is ongoing, while avoiding a significant visual change to the UI. What do you think about this?

But the controls update quickly, right?

The response time for controls and graphs is actually the same, as both are delivered to the client side in a single response from on_page_load. Additionally, there’s no reason why control updates would be faster, as they depend on data_frame loading (sometimes even multiple data frames).
This data_frame loading process is already optimised with #857.

@antonymilne
Copy link
Contributor

antonymilne commented Dec 4, 2024

Just to add on the UI comment: I'm actually not sure it's possible to get something much better here because there's a problem of what to show when the app first loads before you know what any of the options in the selector are. For some selectors like dropdown it might make sense to show a dropdown with no options or a disabled dropdown. But for something like radioitems or checklist there's no prior way to know how big the component will be on the screen because you don't know how many options there will be. So as I see it the options for the visual appearance while it's loading are:

  • show some radioitems with arbritrary values or no values or disabled (but what values should you show?)
  • show nothing - in which case it's nice to have a visual indication that something will load into that space (what we do now)
  • show some other placeholder component like an empty box saying "Loading..." - doesn't seem much better than what we do now

In technical terms, it's almost exactly the same problem as the question of what we show on first page load while a graph/table/figure is loading. Only it's a bit harder because we don't have a fixed grid layout in the control panel and we don't know what size the ultimate loaded component will be in advance, which makes it inevitable (I think?) that there will be some "jumping around" after the selector has loaded, at least in the case of selectors which are not a fixed size like radio items and dropdown.

We could handle cases like fixed size dropdown vs. variable size radioitems differently, but that adds complexity and won't work in all cases.

Personally the current UI is ok for me. I'm also very open to improving it if we can work out how but I'm not sure immediately what would be an improvement? 🤔

@petar-qb petar-qb merged commit c18a6eb into main Dec 4, 2024
36 checks passed
@petar-qb petar-qb deleted the bug/fix_dynamic_dropdown_multi_false_clear_value branch December 4, 2024 13:02
@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Dec 4, 2024

Personally the current UI is ok for me. I'm also very open to improving it if we can work out how but I'm not sure immediately what would be an improvement? 🤔

I haven't given this much thought yet, but an immediate visual improvement that comes to mind is using a single loading visual for the entire control panel e.g. as soon as there is one dynamic control, the entire control panel gets wrapped inside a loading spinner. I am not sure if that is technically feasible? It does make a significant difference to me whether I see multiple loading spinners or just one. It's just a lot of visual noise.

Consider the demo J. showed us this week with over 20 controls. Imagine all of them being dynamic, the UI would be overwhelming if there were 20 small loading spinners in the limited left space 😄 Imagine a combination of dynamic and static now, the UI then looks inconsistent as you would have some loading spinners and some "static space". (This was something I already noticed in the dev example here)

Controls differ from components like Graph, Table, and AgGrid for me. Initially, we debated whether to have a loading spinner for each component or just one for the entire right side as well. We chose individual spinners, thinking users might want to see one component as soon as it finishes loading. Though, I think this argument actually no longer holds, as a component on a page is only visible if all components finished loading? 🤔

Anyway, for controls, this argument is even less relevant. You start interacting with controls, as soon as the entire control panel is ready, so for me it makes more sense to put one loading spinner around the entire control panel.

@maxschulz-COL
Copy link
Contributor

Anyway, for controls, this argument is even less relevant. You start interacting with controls, as soon as the entire control panel is ready, so for me it makes more sense to put one loading spinner around the entire control panel.

Just wanted to second Li's points here - my preference would be one loading spinner for the entire control panel

@petar-qb
Copy link
Contributor Author

petar-qb commented Dec 4, 2024

I created the issue for this -> https://github.com/McK-Internal/vizro-internal/issues/1396
No one have been assigned, but we can decide if we want to include it in Monday's sprint planning.

@antonymilne
Copy link
Contributor

antonymilne commented Dec 5, 2024

Yeah, I can see how having 20 little spinners would be a bit much in such a scenario 😅 It's definitely possible to have a single dcc.Loading for the whole control panel (excluding navigation) - in fact it's easier than the current solution. We don't need to put in any conditional logic to see whether any dynamic filters are contained - we can just wrap the whole control panel (excluding navigation) in dcc.Loading and the loading icon will only ever appear if one of controls is the output of a callback.

The original implementation here was that each dynamic filter components (note plural - this includes the label as well as the dropdown) would be wrapped in dcc.Loading. In #879 @maxschulz-COL suggested that it would be better UX to use wrap only the dropdown in dcc.Loading and leave the label static, which is the current solution.

Controls differ from components like Graph, Table, and AgGrid for me. Initially, we debated whether to have a loading spinner for each component or just one for the entire right side as well. We chose individual spinners, thinking users might want to see one component as soon as it finishes loading. Though, I think this argument actually no longer holds, as a component on a page is only visible if all components finished loading? 🤔

This argument does still hold, at least if you have static components like vm.Card. These display straight away, before any dynamic components have finished loading. This works nicely because it feels like you have the static structure in place and then the user just waits for the dynamic components to load (which then do all load at the same time, not one at a time).

The current solution for controls mirrors exactly this - we keep the static components like labels fixed and the dropdown itself is in dcc.Loading. Like with components, all dynamic controls finish loading at the same time.

However, you're right that controls are not the same as components, and seeing 20 little loading icons on the control panel would be too much. Ultimately I think there are probably two reasonable solutions here: the current solution or a single loading icon over the whole control panel. The current solution is maybe better in some cases (few dynamic controls) and the single loading icon is probably better in some cases (many dynamic controls).

The other thing to bear in mind is what should happen when a control (could be dynamic or static) is updated outside initial page load. e.g. think of the new proposed mechanism for filter interaction where clicking on a graph updates a filter. In this case it feels like worse UX to me for the whole control panel to go into a loading state when only one dropdown is updated.

So ultimately I think maybe there's two options here:

  • current solution but somehow improve UX for page with many dynamic controls e.g. make loading icon smaller or less intrusive or something else like in [Bug] Make single dynamic dropdown clearable #915 (comment) (not sure what though)
  • single loading icon for whole control panel and:
    • either live with the whole panel being put into loading state when only a single dropdown is updated while you're on the page
    • or come up with a workaround here so that the single loading icon is only active during the on page load callback and after that it goes to individual loading icons while you're on the same page (probably possible, I've done similar before, but it does add complexity)

@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Dec 5, 2024

This argument does still hold, at least if you have static components like vm.Card. These display straight away, before any dynamic components have finished loading. This works nicely because it feels like you have the static structure in place and then the user just waits for the dynamic components to load (which then do all load at the same time, not one at a time).

Ah yes, I forgot about the static components. I was thinking more about having a Graph and Table on a page, where they all load simultaneously as you mentioned. However, I completely agree with your point above 👍.

The current solution for controls mirrors exactly this - we keep the static components like labels fixed and the dropdown itself is in dcc.Loading. Like with components, all dynamic controls finish loading at the same time.

Ah okay, this feels a bit odd to me. I would have kept the dcc.Loading around the entire dropdown, if used at all 🤔 Unlike static components, the dropdown label is an integral part of the dropdown itself. If you look at any bootstrap documentation, the label and input are always grouped together e.g. inside form-group or form-check. Unlike a Card, I don't see the label as a separate static component. I also don't see any benefit in displaying the label without the dropdown. For instance, what's the advantage of seeing "Select Country" without the dropdown to choose from? In this sense, it works differently for me than a static Card, where it makes sense for me to display it, because it also works as a stand-alone element.

So ultimately I think maybe there's two options here:

What about not having a loading spinner at all? Did it look so bad? 😬 Coming from a BI tool perspective, having a loading spinner on controls generally looks a bit odd to me. In BI tools, the controls just update, appear, or disappear without any loading component. I'm not sure if we can achieve something as smooth with Dash, but even if not, I don't think it looks that bad without a loading spinner?

The other thing to bear in mind is what should happen when a control (could be dynamic or static) is updated outside initial page load. e.g. think of the new proposed mechanism for filter interaction where clicking on a graph updates a filter. In this case it feels like worse UX to me for the whole control panel to go into a loading state when only one dropdown is updated.

Very good point! So the way how I always imagined it to work was something like this:
https://cloud.google.com/looker/docs/cross-filtering-dashboards

Unlike that example, the filter would already be visible from the start, with just the values updating. No loading spinner.
This is also how it works for their hierarchical filters - the options will be just updated, no blinking, no loading spinner, it just happened silently: https://cloud.google.com/looker/docs/filters-user-defined-dashboards#setting_up_linked_filters

Thinking about it now, I think I even prefer no loading spinner over having one for the control panel 😄. To me, the loading spinner makes more sense for the components, not for the controls. I actually just checked, and I don't think it looks that bad without a spinner?


BTW, a bit related and unrelated to this: @petar-qb @l0uden - you mentioned that you wanted a html div that shows when all the actions have finished as it's easier for the tests to check certain elements then. In this example (top right-side), you can actually see a loading-spinner for exactly that: https://cloud.google.com/looker/docs/cross-filtering-dashboards

I really like it, actually, because we wouldn't rely on every single component having a loading spinner (related to topic above). And this also gives you a better indication if any background actions are still running without having a full notification alert system in place yet (which would be even nicer).

@petar-qb
Copy link
Contributor Author

petar-qb commented Dec 5, 2024

I slightly prefer the "loading icon per dynamic control" approach (or a similar per-control visual indicator), as it ensures consistency with how other dynamic components like graphs, tables, and figures behave. This approach aligns with user expectations and the overall design paradigm.

Practically speaking, it's unlikely for users to see more than 7,8 dynamic controls spinners in the control panel due to the dcc.Loading height. Additionally, not all controls on the left side are always dynamic. Some of them are static and always visible, like buttons, cards and similar static components on the right side of the screeen. So, I think it's more likely for the right side of the screen to contain dozen of dynamic components spinners (like KPI-dashboard example).

Users might also experiment with placing filter components on the right side, where the current solution makes the loading spinner visible, and it sounds right to me.

Given this, my preference is to move forward with a per-control indicator approach. Whether it’s a loading spinner, a shining box, or another visually appealing element 😄

BTW, a bit related and unrelated to this: @petar-qb @l0uden - you mentioned that you wanted a html div that shows when all the actions have finished as it's easier for the tests to check certain elements then. In this example (top right-side), you can actually see a loading-spinner for exactly that: https://cloud.google.com/looker/docs/cross-filtering-dashboards

I really like this 👍 👍 Here's the ticket for that -> https://github.com/McK-Internal/vizro-internal/issues/1399

@petar-qb petar-qb mentioned this pull request Dec 9, 2024
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants