-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Dashboard: disallow clicking on buttons and links in Dashboard disabled mode #4292
Conversation
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.
Using CSS for this seems weird, but if that's working, it's all good man.
:-) Seems so. Do you think it's more future-proof to set |
Fixes #4223 (comment), @ahardin |
Alternatively, we could use a |
Good point. So a conditional fieldset that only wraps when |
Why not? |
@@ -198,6 +198,13 @@ | |||
fill: #9f9f9f; | |||
} | |||
|
|||
// Disallow clicking on all interactive elements | |||
.uppy-Dashboard--isDisabled .uppy-Dashboard-innerWrap { | |||
button, a, input, select, textarea { |
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.
why only button, a, input, select, textarea ? can't we just disable pointer-events for everything, e.g the whole dashboard or all children?
alternatively if we want to make it more fine-grained (e.g. still allow some things to be clicked), then I think it's cleaner to do it in javascript (e.g. if (disabled) return
) so that it won't have unintended consequences
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.
Thought it’s more performant to target interactive elements, those that support the disabled
attribute, than doing .uppy-Dashboard--isDisabled .uppy-Dashboard-innerWrap *
.
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.
what about .uppy-Dashboard--isDisabled .uppy-Dashboard-innerWrap
?
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.
That is currently the case
uppy/packages/@uppy/dashboard/src/style.scss
Lines 176 to 195 in 3e796ee
.uppy-Dashboard-innerWrap { | |
position: relative; | |
display: flex; | |
flex-direction: column; | |
height: 100%; | |
overflow: hidden; | |
border-radius: 5px; | |
opacity: 0; | |
.uppy-Dashboard--isInnerWrapVisible & { | |
opacity: 1; | |
} | |
.uppy-Dashboard--isDisabled & { | |
opacity: 0.6; | |
filter: grayscale(100%); | |
user-select: none; | |
pointer-events: none; | |
} | |
} |
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.
ah ok! The only problem with doing this selective disabling as I see it is that if someone were to add a different button type, e.g. <div role="button">
in the future, then this disabling functionality would stop working and nobody will notice.
Because sometimes our views might have no buttons/inputs, otherwise not a form, and it’s weird (semantically incorrect) to have everything be wrapped in an element meant to group things in a form? |
then i would argue it's also semantically wrong and a bit hacky to suddenly "appear" a fieldset on demand just to be able to disable all form elements. |
So the CSS solution seams like the least problematic? We set Otherwise we can make a rule to wrap all buttons, inputs etc in a |
Do we know how accessibility software respond to it vs the use of the |
Changed to |
| Package | Version | Package | Version | | -------------------- | ------- | -------------------- | ------- | | @uppy/audio | 1.0.4 | @uppy/screen-capture | 3.0.2 | | @uppy/companion | 4.3.0 | @uppy/transloadit | 3.1.1 | | @uppy/core | 3.0.6 | @uppy/xhr-upload | 3.1.0 | | @uppy/dashboard | 3.2.2 | uppy | 3.5.0 | | @uppy/locales | 3.0.6 | | | - @uppy/transloadit: fix `assemblyOptions` option (Antoine du Hamel / #4316) - meta: Remove Robodog advice, since it is deprecated (Artur Paikin) - @uppy/dashboard: fix dashboard acquirers list (Mikael Finstad / #4306) - @uppy/dashboard: Dashboard: disallow clicking on buttons and links in Dashboard disabled mode (Artur Paikin / #4292) - @uppy/audio,@uppy/core,@uppy/dashboard,@uppy/screen-capture: Warn more instead of erroring (Artur Paikin / #4302) - @uppy/locales: Update de_DE.js (Jörn Velten / #4297) - meta: use load balancer for companion in e2e tests (Mikael Finstad / #4228) - @uppy/companion: @uppy/companion upgrade grant dependency (Scott Bessler / #4286) - @uppy/xhr-upload: add `'upload-stalled'` event (Antoine du Hamel / #4247) - @uppy/locales: minor enhancements and typo fixes for the hungarian translation (KergeKacsa / #4282)
| Package | Version | Package | Version | | -------------------- | ------- | -------------------- | ------- | | @uppy/audio | 1.0.4 | @uppy/screen-capture | 3.0.2 | | @uppy/companion | 4.3.0 | @uppy/transloadit | 3.1.1 | | @uppy/core | 3.0.6 | @uppy/xhr-upload | 3.1.0 | | @uppy/dashboard | 3.2.2 | uppy | 3.5.0 | | @uppy/locales | 3.0.6 | | | - @uppy/transloadit: fix `assemblyOptions` option (Antoine du Hamel / transloadit#4316) - meta: Remove Robodog advice, since it is deprecated (Artur Paikin) - @uppy/dashboard: fix dashboard acquirers list (Mikael Finstad / transloadit#4306) - @uppy/dashboard: Dashboard: disallow clicking on buttons and links in Dashboard disabled mode (Artur Paikin / transloadit#4292) - @uppy/audio,@uppy/core,@uppy/dashboard,@uppy/screen-capture: Warn more instead of erroring (Artur Paikin / transloadit#4302) - @uppy/locales: Update de_DE.js (Jörn Velten / transloadit#4297) - meta: use load balancer for companion in e2e tests (Mikael Finstad / transloadit#4228) - @uppy/companion: @uppy/companion upgrade grant dependency (Scott Bessler / transloadit#4286) - @uppy/xhr-upload: add `'upload-stalled'` event (Antoine du Hamel / transloadit#4247) - @uppy/locales: minor enhancements and typo fixes for the hungarian translation (KergeKacsa / transloadit#4282)
Before:
After: