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

[Feat] Update to use dmc 0.15.1 #924

Merged
merged 17 commits into from
Jan 20, 2025
Merged

[Feat] Update to use dmc 0.15.1 #924

merged 17 commits into from
Jan 20, 2025

Conversation

AnnMarieW
Copy link
Contributor

@AnnMarieW AnnMarieW commented Dec 7, 2024

This is a draft PR. For details, please refer to the discussion in #905

TO DO:

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.

@AnnMarieW AnnMarieW mentioned this pull request Dec 7, 2024
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.

First, thank you so much, @AnnMarieW, for this PR and your detailed explanations here! 🚀 This is incredibly helpful.

We definitely plan to upgrade the dmc version, and although we've removed other dmc components, we'll likely always keep the DatePicker since it's the best available! 💯

I've left a few comments, but there's no rush to implement them. We probably won't merge this PR until next year. We were already planning to upgrade dmc next year, but we wanted to check feasibility first. Your PR is fantastic for that! Everything seems to work fine, and it looks very feasible and straightforward from a code perspective. Thank you so much! 🙏

I'll have @nadijagraca and @petar-qb do some manual checks since they developed the component back then and probably remember better than me all the workarounds we added 😅 @nadijagraca, I'll create a separate ticket, but we need to change our CSS to match the new DatePicker CSS selectors, but that should be straight-forward hopefully!

Overall, I'm confident we can merge this PR next year though 👍 🚀

vizro-core/examples/dev/app.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/_vizro.py Outdated Show resolved Hide resolved
@AnnMarieW
Copy link
Contributor Author

Hi @huong-li-nguyen

Thanks for the review and I look forward to working on this more next year!

need to change our CSS to match the new DatePicker CSS selectors, but that should be straight-forward hopefully!

As far as styling goes -- it's changed quite substantially starting in V0.14. No need to go into details now, but a couple things to note:

  • Setting the theme prop in MantinePriverder will make it so that any custom component can also have the Vizro default themes. For example setting the theme={"primarColor: "dark"} is very close to the Vizro theme and works well in both light and dark modes. This will reduce the amount of custom CSS needed.

  • by default the calendar dropdown is rendered in a portal which is outside the dom tree. This means it can't be targeted with the component's className prop. However, it can be styled using Mantine's Style API. More on this later :-)

Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

@AnnMarieW, thank you for such great work ❤️ I really like dmc.DatePickerInput!

I left a few comments, but this PR is close to being merged in my opinion 🎉

P.S. You can paste the code from the PR description you wrote directly to vizro-core/examples/scratch_dev/app.py file, and make it easier for other reviewers to test the change 😃.

vizro-core/examples/dev/app.py Outdated Show resolved Hide resolved
vizro-core/pyproject.toml Outdated Show resolved Hide resolved
vizro-core/src/vizro/_vizro.py Show resolved Hide resolved
vizro-core/src/vizro/_vizro.py Show resolved Hide resolved
vizro-core/src/vizro/models/_dashboard.py Outdated Show resolved Hide resolved
@petar-qb
Copy link
Contributor

@AnnMarieW Can you paste the code from the PR description you wrote, directly to vizro-core/examples/scratch_dev/app.py file? This would make it easier for other reviewers to test the change 😃.

@petar-qb
Copy link
Contributor

Unit tests should not be too hard to fix. There are only three tests that are failing:

  1. test_datepicker_build (2 - for range=False and range=True) (file: test_date_picker.py),
  2. test_dashboard_build (file: test_dashboard.py)

You can fix test_datepicker_build by changing the expected_datepicker to the new form: dmc.DatePickerInput(...). Similarly, for the test_dashboard_build, all you need is to wrap the expected_dashboard_container inside the dmc.MantineProvider(expected_dashboard_container).

You can run unit tests with the following command from your terminal (ensure to call it from .../vizro/vizro-core location): hatch run test-unit.

If you need any help with this, please let us know and we will be happy to jump in for the help. 😃

@AnnMarieW
Copy link
Contributor Author

This commit is to show how to style Mantine componets with a Vizro style.

It uses the theme prop in the MantineProvider to make the default accent color gray, which works well with VIzro. This will be helpful for users adding custom Mantine components in the future.

To fine tune the datepicker, it's best to use Mantine's Style API. I deleted the datepicker.css file because most of the selectors have changed. Also, it's not possible to style the calendar with the className prop because it's rendered in a portal that is outside of the component root and inner elements are not part of the component tree. We can change it to withinPortal=False, but that can cause some issues with the z-Index.

The ideal way to style the calendar is to use Mantine Styles API. I included an example of the styles prop making all the days the same color to remove the default red for weekend days.

@petar-qb petar-qb mentioned this pull request Dec 19, 2024
1 task
@petar-qb
Copy link
Contributor

petar-qb commented Jan 7, 2025

@AnnMarieW thanks for writing the unit tests. They work like a charm. 🎉

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.

Hello @AnnMarieW, great to see you here again! Sorry for the very slow review - I just back after an extended Christmas break this week. Hope you had a wonderful festive period too. And thanks very much @petar-qb and @huong-li-nguyen for all the reviewing and comments so far.

Thank you so much for your comments in #905 and all your work here. I'm super happy to hear that you're now the primary maintainer of dmc and even happier that you raised a PR to do the version bump for us! Even though we have migrated almost everything on vizro to dbc and only use the date picker from dmc now, as you know I am a huge fan of dmc and hope it goes from strength to strength.

FYI I saw that you were running hatch run examples:example dev. Actually to run the dev example you just need to do hatch run example dev and for the scratch_dev example (which we usually use for testing) it's just hatch run example. I updated our contributing guidelines a couple of months ago so hope they're easier to follow now.


I think we should make this PR ready to review now. I left a few comments. There's a few other things I think we need to deal with but nothing major. Once we've figured these remaining bits out I'd be very happy to approve and get this merged :shipit:

  • @AnnMarieW cfb1c3b - is this meant to remain or was it just to demonstrate something?
  • @AnnMarieW until now it's been possible to run vizro entirely offline, but in this PR you've added a dependency to https://unpkg.com/@mantine/[email protected]/styles.css through dmc.styles.DATES. Would it be a bad idea for us to just ship this file with vizro? Is the file likely to change much in future? Because then we would have to manually update our file to match.
  • @AnnMarieW please could you point me to where I can read more about this? I didn't see it in the dmc docs anywhere. "Also, it's not possible to style the calendar with the className prop because it's rendered in a portal that is outside of the component root and inner elements are not part of the component tree. We can change it to withinPortal=False, but that can cause some issues with the z-Index."
  • @petar-qb so nice to see all those random hacks gone! This is a HUGE impovement 🎉 As things stand after this PR, does the date picker persistence work the same as our other selectors or still not as good?
  • @huong-li-nguyen @nadijagraca let's have a quick chat about the best way to handle the CSS

vizro-core/examples/dev/app.py Show resolved Hide resolved
vizro-core/pyproject.toml Outdated Show resolved Hide resolved
vizro-core/src/vizro/_vizro.py Show resolved Hide resolved
vizro-core/src/vizro/models/_dashboard.py Show resolved Hide resolved
@petar-qb
Copy link
Contributor

@antonymilne

@petar-qb so nice to see all those random hacks gone! This is a HUGE impovement 🎉 As things stand after this PR, does the date picker persistence work the same as our other selectors or still not as good?

After @AnnMarieW resolved this #924 (comment), persistence started to work completely correctly.

@AnnMarieW
Copy link
Contributor Author

AnnMarieW commented Jan 16, 2025

@antonymilne

Welcome back! I'm happy to be working on another contribution to Vizro 🙂
I've answered all the other questions in-line, I think there is only this one question about styling components with a portal remaining:

please could you point me to where I can read more about this?

The best description is in the upstream Mantine docs for the Portal component. This component is not available as a separate component in DMC because it's used as a building block for other components like Modals, Select, Popover, and the dropdown calendar of date picker components.

The bit about the z-index, I think I read about in an issue on the Mantine Discord. I'll look for more info. It may be that it's ok to use this component with or without the portal. Either way, the bulletproof way to style this component is with the styles API.

Update
Nope - one more question:

Until now it's been possible to run vizro entirely offline, but in this PR you've added a dependency to https://unpkg.com/@mantine/[email protected]/styles.css through dmc.styles.DATES. Would it be a bad idea for us to just ship this file with vizro? Is the file likely to change much in future? Because then we would have to manually update our file to match.

It would be good to include the dmc.styles.DATES locally. The only issue is that these styles do update occasionally when changes are made in the upstream Mantine library. There would be an extra step to update the stylesheets when changing versions, but it shouldn't be too burdensome. I can help keep an eye on this too.

@petar-qb
Copy link
Contributor

Hey people @AnnMarieW @antonymilne @huong-li-nguyen @nadijagraca 👋.

After the second review, unfortunately, I found two unexpected behaviours in the new dmc=0.15.0 for the type="range" dmc.DatePickerInputcomponent. Both are described in the PR description.

For easier visibility, I'm posting this PR's description here too (the first issue, that's solved with the linked PR also happens in the dmc==0.12.0):


If a user selects one of two values in the type="range" - dmc.DatePickerInput, and then clicks somewhere else on the page, [null, null] will be set in the component. This PR (the linked one) enables that the previously selected value (that was correct e.g. ["2025-01-01", "2025-05-05"]) overwrites the [null, null] when this happens. So the bug is solved.


There's another problem that I wasn't able to solve with the javascript code or dmc.DatePickerInput configuration. So, when user selects the first value for the type="range" - dmc.DatePickerInput, all callbacks where this component is a dcc.Input will be triggered. Component value in that case is for example ["2025-01-01", null]. Vizro in that case return an empty data_frame if this component is used as a vm.Filter (so this partially solves the problem).

So, selecting a complete value for this component means two requests to the server which is a bit odd:

  1. The first request when only one value is selected (then an empty data_frame is returned for all its filter targets).
  2. The second request when the second value is selected (then it works properly).

This problem (that I wasn't able to solve) happens with the dmc==0.15.0 but did not happen with the dmc==0.12.0.
For the dmc==0.12.0, callbacks are triggered only after the second value is selected.


My opinion is that we should merge this PR fix.
Also, I don't think that the second issue (sending 2 requests to the server) is something that should stop us for merging this PR ("Update to use dmc 0.15.1"). I described it just in case you @AnnMarieW were not aware of this behaviour. Fixing this on the dmc side will automatically solve this unexpected behaviour on the Vizro side too.

@AnnMarieW
Copy link
Contributor Author

To address these issues in this comment #924 (comment) could try setting the following props:

debounce=True

debounce (boolean | number; default False): If True, changes to input will be sent back to the Dash server only on enter or when losing focus. If it's False, it will send the value back on every change. If a number, it will not send anything back to the Dash server until the user has stopped typing for that number of milliseconds.

allowSingleDateInRange=False

allowSingleDateInRange Determines whether single year can be selected as range, applicable only when type="range".

@petar-qb
Copy link
Contributor

To address these issues in this comment #924 (comment) could try setting the following props:

debounce=True

debounce (boolean | number; default False): If True, changes to input will be sent back to the Dash server only on enter or when losing focus. If it's False, it will send the value back on every change. If a number, it will not send anything back to the Dash server until the user has stopped typing for that number of milliseconds.

allowSingleDateInRange=False

allowSingleDateInRange Determines whether single year can be selected as range, applicable only when type="range".

Hi @AnnMarieW, I've already tried this combination of properties and the first request when only one value is selected does not really happen 👍. However, after the second value is selected, the user has to click somewhere else on the screen again to trigger the request. This behaviour seemed like a worse UX practice than sending the request twice, so I left debounce=False and allowSingleDateInRange=True.

I really think we should merge this PR that fixes the case when the value is cleared (which can happen if one value is selected and then the user clicks somewhere else on the screen). This partially improves the UX.

Also, as I mentioned in #924 (comment), I think we should merge this PR even though two requests are sent to the server. If in a newer version of dmc it stops sending the first request after the first value is selected, we will be happy to update the dmc version to a newer version in Vizro. 😃

@huong-li-nguyen huong-li-nguyen changed the title Update to use dmc 0.15.1 [Feat] Update to use dmc 0.15.1 Jan 20, 2025
@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Jan 20, 2025

I really think we should merge this PR that fixes the case when the value is cleared (which can happen if one value is selected and then the user clicks somewhere else on the screen). This partially improves the UX.

Also, as I mentioned in #924 (comment), I think we should merge this PR even though two requests are sent to the server. If in a newer version of dmc it stops sending the first request after the first value is selected, we will be happy to update the dmc version to a newer version in Vizro. 😃

On the first point, I actually don't think we need to fix it. I think the UI is fine as is, but let's just take this offline and discuss with @Joseph-Perkins. Either way, we can add that fix after this PR if we decide to do that 👍

So for me, this PR is ready to merge @AnnMarieW 👍 Let's merge that together with the CSS PR after @antonymilne final approval. For linting to pass @AnnMarieW , you would have to: @antonymilne did this already (see below)

  • Add a changelog file via hatch run changelog:add
  • Remove the datepicker.css file completely

@huong-li-nguyen huong-li-nguyen marked this pull request as ready for review January 20, 2025 10:08
@huong-li-nguyen huong-li-nguyen self-requested a review January 20, 2025 10:11
@huong-li-nguyen huong-li-nguyen linked an issue Jan 20, 2025 that may be closed by this pull request
@antonymilne
Copy link
Contributor

antonymilne commented Jan 20, 2025

@AnnMarieW no need to do the things that @huong-li-nguyen asked for above - I've put them into my commits. Will push it all and what we still need to do when I get a chance this afternoon, but for now you don't need to do anything 🙂

Edit: Unfortunately Github didn't let me push the commits onto your fork so we'll do it in a separate PR instead. The plan is:

  • merge this PR to main
  • move our three PRs from https://github.com/AnnMarieW/vizro/pulls into this repo so they're easier to manage
  • merge my one of those ASAP since it's really meant to be part of this PR
  • I'll take a look at the styling one and @petar-qb's

Thank you to everyone for all your work on this and especially @AnnMarieW!

@antonymilne antonymilne merged commit c7cb04d into mckinsey:main Jan 20, 2025
32 of 33 checks passed
@AnnMarieW
Copy link
Contributor Author

AnnMarieW commented Jan 20, 2025

Also, as I mentioned in #924 (comment), I think we should merge this #924 even though two requests are sent to the server. If in a newer version of dmc it stops sending the first request after the first value is selected, we will be happy to update the dmc version to a newer version in Vizro. 😃

For the debounce on the datepicker, it is working as intended. It updates on blur, allowing the user to select various dates, then only update once when they have decided on the date. I find when selecting a range it sometimes takes a couple tries to set it correctly, or to change the range. If you prefer something that feels a little more responsive, the setting debounce=False and then checking for two dates in the callback, is the way to go. Feel free to open a discussion in the DMC GIthub if you would like the debounce to work differently. If it's a popular request, we could make that an option for the debounce.

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.

Try to bump dmc
4 participants