-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#8769] Kiezradar: Allow creation and editing of kiezradars #5993
Conversation
65cfb2c
to
b76a97f
Compare
meinberlin/apps/kiezradar/templatetags/react_kiezradar_filters.py
Outdated
Show resolved
Hide resolved
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.
Thanks for the first part of that story! Great work, I just left a few remarks where you might want to have a second look.
|
||
export default function Loading () { | ||
return ( | ||
<div className="kiezradars__loading" aria-label={loading}> |
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 you think we need aria-live or some way to hand live status here? @vellip your thoughts?
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.
@hom3mad3 I added aria-live="polite"
, what do you think?
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.
aria-live
should exist at the top level, not the loading indicator. It only announces when descendants change so putting it on a loading indicator will not announce any change as it will just always be the same text. You will want to...
- set the innermost dom node around dynamic content in a component that does fetching to
aria-live="polite"
return (
<>
<h1>Title</h1>
<p>Text</p>
<div aria-live="polite">
{data.length && (
<ul>
{data.map(i => <li>{i}</li>)}
</ul>
)}
</div>
</>
)
- have a loading indicator within the live-region
- have that loading indicator not be inserted on first render (because a live-region only announces changes to the DOM)
- instead have it be rendered in once you start the fetching in the useEffect (basically, initialise
isLoading
withfalse
and callsetIsLoading
in theuseEffect
when fetching)
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.
@vellip thank you for the explanation, i learned a lot ❤️
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.
Thanks @vellip, this is now updated 👍
3971adb
to
165af47
Compare
d105888
to
4006f63
Compare
4006f63
to
871a062
Compare
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.
@sevfurneaux thank you for the great work and patience with all those comments, it looks great! ✨ 🤗
A pleasure! Thanks for both your comments, @hom3mad3 and @vellip: learnt a lot 🫶 |
@vellip, when you have some time, could you give it a thumbs-up? I don’t want to dismiss your change for request just so I can merge. Even though we don’t have a code of conduct with specific rules (which isn't ideal), I want to make sure we're being considerate and giving everyone a final word. |
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.
Looks good, thank you so much!
same here! 📙🤓 |
Describe your changes
This PR adds the kiezradar filter list, new and editing pages.
Tasks