Fix dropdowns and logout #4056
Fix dropdowns and logout #4056
Conversation
| @@ -31,7 +26,11 @@ | |||
| border: 1px solid var(--colors-grey-200); | |||
| background-color: var(--colors-white); | |||
| max-height: 90vh; | |||
| overflow-x: scroll; | |||
There was a problem hiding this comment.
This was added for a reason. (eg, very long lists of organizations)
There was a problem hiding this comment.
Yes, but we do not need to scroll in the x direction
There was a problem hiding this comment.
Hm true, that should have been overflow-y
Odd that it seems to have worked anyway, probably because scroll is on by default..
| max-width: 50%; | ||
| width: auto; |
There was a problem hiding this comment.
Are you sure you need to remove all of these lines to make the clicking work again? It seems unlikely.
There was a problem hiding this comment.
There were a lot of inconsistencies when it comes to width, having the items in the dropdown to expand for a and buttons they must expand to full width and where the min width is the width of the max-content.
| {{ lang.name_local.title }} | ||
| </button> | ||
| </a> | ||
| <button form="set-language" type="submit" name="language" value={{ language }}> |
There was a problem hiding this comment.
Still dont understand why our language switching is handled by a form, and not just a link.
There was a problem hiding this comment.
I think this is how it should be done according to the Django docs
There was a problem hiding this comment.
I assume because we're using Django's set_language redirect view that handles this switch. According to the docs: The view expects to be called via the POST method, with a language parameter set in request.
There was a problem hiding this comment.
Here is an explanation:
https://github.com/django/django/blob/stable/5.1.x/django/views/i18n.py#L30
There was a problem hiding this comment.
POST requests are for State changes.
A switch to a different language is not a state change (or shoudn't be) as the used language is present in each url, and cannot rely on stateful magic to select the language anyway.
For example, what would be the state if you where to select English. And then go to an url with dutch listed as the language. Would that GET url magically do the same state change, or does the language not reply on state, but instead just rely on the language listed in the url?
| <form id="set-language" | ||
| action="{% url "set_language" %}" | ||
| method="post" | ||
| class="inline"> |
There was a problem hiding this comment.
I guess the form could be class="hidden" as it contains no elements anyway?
There was a problem hiding this comment.
I moved this code together with the language selector button. It has 1 button in it that submits the value or we can create an input type hidden to set the value.
| <div class="collapsing-element"> | ||
| <div class="dropdown"> | ||
| <button type="button" | ||
| aria-controls="user-profile" |
There was a problem hiding this comment.
We seem to have lost the data-open en data-close labels?
There was a problem hiding this comment.
Ahh, yes, I will add them again. This code is a bit old as we were waiting for the new Manon. I don't know if this is the case now, or I can just add it for the new styling of manon. I know that there were some differences in rocky why we use buttons. reference: https://minvws.github.io/nl-rdo-manon/components/collapsible
There was a problem hiding this comment.
Now I know why we choose the button styling. using styling of Manon we can only add text for data-open-label and data-close-label Here is an example:
The text will be added as dropdown text, but we use icons as well, the alternative was to use buttons instead so we can have more flexibility. But I think we can find a mid way between both. I think I can just add these labels and change text when it opens for screen readers.
There was a problem hiding this comment.
@ppvg could you shine your light on this?
There was a problem hiding this comment.
I'm not sure I know what the question is so I'd have to look into this.
I can say though that:
- The reason for Manon's dropdown to rely on
data-attributes for the button label was for progressive enhancement, i.e. so that Manon can "inject" the button if (and only if) JS is available. - I'm not convinced changing the label was a good idea, and it would probably be better to use the label only to identify (e.g. "profile menu") and use ARIA attributes to convey state (e.g.
aria-controls+aria-expanded=true/false). We should probably revise Manon's dropdowns in a future version.
…ination into fix/dropdown-profile
|
|
closes #3402 |
Changes
Issue link
#4055
Closes #4055
Demo
Screen.Recording.2025-01-30.at.14.08.24.mov
QA notes
you have to run yarn to test out these changes:
cd rockynvm use 18yarnyarn devCode Checklist
.envchanges files if required and changed the.env-distaccordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.