-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
a11y: use role=alert for messages from Django and JS #1641
Conversation
b98de4d
to
6d47382
Compare
The only place where we set a message from views is when we clone a map. |
3616311
to
9a2b4fd
Compare
The |
Also define a custom module+css for alerts (chore).
9a2b4fd
to
d6e4da3
Compare
There are two remaining complex cases where there are buttons and logic within the alert:
|
@@ -0,0 +1,54 @@ | |||
export default class Alerts { | |||
constructor() { | |||
this.alertNode = document.querySelector('[role="alert"]') |
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.
nit: Maybe alertNode
could be a private property of the object?
} | ||
|
||
add(message, level = 'info', duration = 3000) { | ||
this.alertNode.innerHTML = ` |
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 this replacing all the whole node? How does this plays with multiple alerts being added?
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's indeed a concern, I was wondering if that case actually happens 🤔
|
||
_display(alert) { | ||
const duration = alert.dataset?.duration || 3000 | ||
const level = alert.dataset?.level || '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.
This repeats (in the code) the default behavior values. Maybe use a constant tied to the class instead?
` | ||
const alertDiv = wrapper.firstElementChild | ||
this.alertNode.after(alertDiv) | ||
if (isFinite(duration)) { |
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.
Love using Infinity
+ isFinite
🤝
Also define a custom module+css for alerts (chore).
This is an early stage, more a discussion at this point.
It works with messages from Django and set dynamically via JS.
The old code is still present.
See: