Bugfix: scope CSS of input elements #2589
Conversation
components/dash-core-components/tests/integration/input/test_input_basics.py
Outdated
Show resolved
Hide resolved
| @@ -54,15 +54,6 @@ | |||
| ], | |||
| "description": "Indicates whether controls in this form can by default have their values automatically completed by the browser." | |||
| }, | |||
| "autoFocus": { | |||
There was a problem hiding this comment.
weird, autoFocus disappeared from the reference docs? Yes: mdn/content#27734 - the PR mentions this page: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes which (a) we don't inspect at all during the build process, (b) implies autoFocus applies to a lot more than just these four element types.
This seems like it would be a breaking change - I don't actually know if this attribute works in a Dash app, we may be adding our elements too late in the page lifecycle for this to be honored, but even if it doesn't work we shouldn't break an app that already includes it.
More broadly, I don't like the fact that in every PR our tests are at the mercy of MDN's changes, we should really make "update the HTML spec from MDN" an optional thing that we do occasionally, maybe when we're updating dependencies or maybe even less frequently. @T4rk1n thoughts?
There was a problem hiding this comment.
I think we should change the generation to be more stable. The test allow us to see there is change, but maybe it should be a CI job that run once a month or so and create a PR if there is change.
An alternative could be to change the scraper and instead use the React types that are contained in JSX.IntrinsicElements. That would change only when the react version is changed and should more stable.
There was a problem hiding this comment.
It looks like the changes to attributes.json don't actually need to be committed now. I reverted them, and the build/tests still function without them.
However, the change to extract-elements.js does need to be here in order to complete a build.
So I'd propose decoupling attributes.json from this PR, and we can think about this as a separate fix.
There was a problem hiding this comment.
I think we'll need to address this prior to a release regardless, as that process will call the full rebuild. But yeah for the sake of this PR we can ignore it.
| {...omit( | ||
| [ | ||
| 'className', |
There was a problem hiding this comment.
FYI we use this a lot but I'd love to get rid of this ...omit pattern pretty much wherever we use it. It causes various problems, like I bet we're currently improperly forwarding persistence/persistence_type/persisted_props here, and some other places we still forward loading_state so the DOM ends up with `loading_state="[object Object]". Instead we should figure out which props we DO want and pick them, or just individually forward them. I thought I had made an issue for this but not seeing it now 🤔
This PR adds a class name to
inputelements so that the CSS for those inputs can be contained within the dash app and not affect non-Dash inputs.Contributor Checklist
optionals
CHANGELOG.md