-
Notifications
You must be signed in to change notification settings - Fork 148
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
Update to use dmc 0.15.1 #924
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this 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 👍 🚀
Thanks for the review and I look forward to working on this more next year!
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:
|
Co-authored-by: Li Nguyen <90609403+huong-li-nguyen@users.noreply.github.com>
for more information, see https://pre-commit.ci
There was a problem hiding this 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 😃.
@@ -21,7 +21,7 @@ dependencies = [ | |||
"pandas>=2", | |||
"plotly>=5.12.0", | |||
"pydantic>=1.10.16", # must be synced with pre-commit mypy hook manually | |||
"dash_mantine_components<0.13.0", # 0.13.0 is not compatible with 0.12, | |||
"dash_mantine_components==0.15.1", # 0.13.0 is not compatible with 0.12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to https://iscinumpy.dev/post/bound-version-constraints/, I think we should remove the upper-bound version constraint (actually the version pinning in this case).
"dash_mantine_components==0.15.1", # 0.13.0 is not compatible with 0.12, | |
"dash_mantine_components>=0.15.1", |
Or maybe:
"dash_mantine_components==0.15.1", # 0.13.0 is not compatible with 0.12, | |
"dash_mantine_components>=0.15.0", # 0.13.0 is not compatible with 0.12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With DMC I think it's best to pin an exact version. We are not yet at a major release (that's coming soon), plus there will be a breaking change when Dash 3.0 comes out that will require an update to DMC.
Whenever this gets merged, I can help with managing dependencies (at least until we get to a major release)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great! Thank you 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to do dash_mantine_components~=0.15.1
so we get updates like 0.15.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, I was a huge fan of ~=X.Y
, ~=X.Y.Z
, <=X
notations, but after reading this article I changed my mind a bit. Now, I'm not to strict about the silver bullet solution for this topic so I also give a chance to the >=X.Y.Z
notation.
Actually, for this case, I prefer this (>=X.Y.Z
) notation because we are not sure that the dmc==0.16.0
will break the current dmc.DatePickerInput
(0.15.0 ). I prefer using ~=
, ==
or <=
notation only if it's much more likely that the new version will break something. Otherwise, let it update, and if something goes wrong (which doesn't happen as ofter as I though before), we can easily pin the lower version until Vizro become adjusted to the breaking change. Guided by those thoughts, I guess we should leave dash_maintain_components>=0.15.1
, but also pin the dash version to dash<3.0.0
(as we already did), because it's more likely that dash==3.0.0 will break something in Vizro .
dash._dash_renderer._set_react_version("18.2.0") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this comment, you said that:
Mantine V7 requires React 18, which will be the default in Dash 3.0. For now, React 18 needs to be explicitly set in the app or via an environment variable.
I'm missing something here. I see that _set_react_version("18.2.0")
is suggested even in the official README.md from the dmc repo.
However, I'm a bit confused. Let's say that the following dependency is set in Vizro -> "dash_mantine_components>=0.15.1"
. Isn't that "react==18.2.0"
will be automatically installed as dmc==0.15.1
has that constraint in its package.json
file? Why should we handle any of dependency versions explicitly from the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the React version used at runtime is controlled by the Dash renderer. The current default is React v16.14.0.
Mantine requires a minimum of React 18, which is why it needs to be set explicitly. This won't be necessary in Dash 3.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oke 👍 It would be helpful if you can add a comment to consider removing this line when we increase the dash dependency to >=3.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setting this react version likely to cause problems with anything else? I don't know whether some Dash components only function with React v16.14.0?
for more information, see https://pre-commit.ci
@AnnMarieW Can you paste the code from the PR description you wrote, directly to |
Unit tests should not be too hard to fix. There are only three tests that are failing:
You can fix You can run unit tests with the following command from your terminal (ensure to call it from If you need any help with this, please let us know and we will be happy to jump in for the help. 😃 |
This commit is to show how to style Mantine componets with a Vizro style. It uses the 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 The ideal way to style the calendar is to use Mantine |
vizro-core/tests/unit/vizro/models/_components/form/test_date_picker.py
Outdated
Show resolved
Hide resolved
@AnnMarieW thanks for writing the unit tests. They work like a charm. 🎉 |
There was a problem hiding this 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
- @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/dates@7.14.3/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
target="_blank", | ||
className="anchor-container", | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand: why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously it was like this:
app.layout = html.Div([...])
Now it's like
app,layout= dmc.MantineProvider(html.Div([...])
So, previously, the app.layout.children was a list, and now it's a single component. If it's a common pattern to add components to the layout like in this app, then we should make sure that children is a list, like below:
app.layout = dmc.MantineProvider([html.Div[]])
@@ -21,7 +21,7 @@ dependencies = [ | |||
"pandas>=2", | |||
"plotly>=5.12.0", | |||
"pydantic>=1.10.16", # must be synced with pre-commit mypy hook manually | |||
"dash_mantine_components<0.13.0", # 0.13.0 is not compatible with 0.12, | |||
"dash_mantine_components==0.15.1", # 0.13.0 is not compatible with 0.12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to do dash_mantine_components~=0.15.1
so we get updates like 0.15.2?
dash._dash_renderer._set_react_version("18.2.0") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setting this react version likely to cause problems with anything else? I don't know whether some Dash components only function with React v16.14.0?
className="datepicker", | ||
**date_range_picker_kwargs, | ||
# removes the default red color for weekend days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want this or was it just to demonstrate something? We don't normally do inline styles like this. @huong-li-nguyen @petar-qb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AM added this to demonstrate how to update some of the styles. I would ignore all CSS changes in this branch and wait for @nadijagraca PR today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to do dash_mantine_components~=0.15.1 so we get updates like 0.15.2?
Yes, I think that would be OK
Is setting this react version likely to cause problems with anything else? I don't know whether some Dash components only function with React v16.14.0?
Plotly has been testing with React 18 for over a year. There hasn't been any issues that I know of. I was just reminded that Dash 3.0 will be using React 18.3.1, and in that version there will be a deprecation warning in the browser console for any components that use the defaultProps
function. Both DMC
and DBC
and Dash AG Grid
are on a path to fix this. I'm not sure about any other community libraries. The Dash component libraries, dcc
, html
and DataTable
have been updated. Pinning it to React 18.2.1 would be good until the other libraries are ready.
Do we actually want this or was it just to demonstrate something? We don't normally do inline styles like this
It would be better if the styles are applied in the theme prop in the MantineProvider
, then it would also apply to any custom DMC component that was added.
I'd be happy to join a call to discuss CSS if you like. Mantine has a different way to style apps and components than Bootstrap, and I could help highlight the differences and leverage the Mantine system to make it easier to maintain going forward. In the meantime, here are a few links that might help:
- Mantine API Overiew - great starting point for understanding styling in DMC>=0.14
- Forum Post on DMC theme builder - When I started playing with this app I had an "ah-ha" moment on the power of the Mantine theme object which sets global style defaults.
- Styles API - how to set component level styles either globally or on individual components.
- The whole "Theming" category in the docs has been updated recently and has lots of great info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nadijagraca has opened a PR here: AnnMarieW#1
From my side, it has all the visual changes we need at the moment.
@nadijagraca - could you check the links above and see whether we can refactor our CSS some more? :) For example, it seems that we could set a border-radius of 0 for all dmc components on the Mantine theme object etc.
@AnnMarieW - feel free to take a look at that PR as well 👍
I also don't want to drag this PR for too long, so from my side, we can merge and refactor the CSS in another PR.
@@ -171,6 +172,11 @@ def build(self): | |||
dash.page_container, | |||
], | |||
) | |||
return dmc.MantineProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this? I read through the dmc docs and played around and as far as I can tell the only thing that dmc.MantineProvider
does is set <head data-mantine-color-scheme=...>
. You already added a line to set this variable in update_dashboard_theme
, which always runs when the page is first loaded. Hence unless dmc.MantineProvider
does something in addition to setting <head data-mantine-color-scheme=...>
this would seem redundant because it just gets overidden straight away by the update_dashboard_theme
callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the MantineProvider
does more than just set the theme. It's required as of DMC 14.0 (Mantine 7) No Mantine component will render if the MantineProvider
is not included in the app. More info in Mantine API Overiew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from that, it seems like we need it anyway. I've added the font-family
to @nadijagraca CSS PR, as otherwise, you would have to set it in several classes. This seemed to be the cleanest way to apply the global font to all dmc components that will be potentially added.
dmc.MantineProvider(
layout,
# Applies to all Mantine components
theme={"fontFamily": "Inter, sans-serif, Arial, serif"},
)
After @AnnMarieW resolved this #924 (comment), persistence started to work completely correctly. |
Co-authored-by: Antony Milne <49395058+antonymilne@users.noreply.github.com>
Welcome back! I'm happy to be working on another contribution to Vizro 🙂
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
It would be good to include the |
Hey people @AnnMarieW @antonymilne @huong-li-nguyen @nadijagraca 👋. After the second review, unfortunately, I found two unexpected behaviours in the new 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 If a user selects one of two values in the There's another problem that I wasn't able to solve with the javascript code or So, selecting a complete value for this component means two requests to the server which is a bit odd:
This problem (that I wasn't able to solve) happens with the My opinion is that we should merge this PR fix. |
To address these issues in this comment #924 (comment) could try setting the following props:
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 Determines whether single year can be selected as range, applicable only when type="range". |
This is a draft PR. For details, please refer to the discussion in #905
TO DO:
type="range" DatePickerInput
is cleared (find more in the PR description) -> Fix when range date_picker is cleared AnnMarieW/vizro#2Test Code
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":