XWIKI-23332: The width of the required rights modal depends on the displayed rights #4322
Conversation
…on the displayed rights * Measure the width of the rights using JavaScript. * Switch to the vertical layout in JavaScript based on the available vs. required width.
michitux
changed the title
| /** | ||
| * @returns {number} the minimum width of the rights list in horizontal layout | ||
| */ | ||
| #computeMinWidth() |
There was a problem hiding this comment.
I'm curious about this # prefix. Is it a new naming rule I missed?
There was a problem hiding this comment.
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_properties - I'm not sure if we've used them yet but to me, it seemed logical to use them as they are relatively widely supported.
There was a problem hiding this comment.
Indeed, I'm used to the private typescript keyword, but indeed the JavaScript equivalent seems widely adopted https://caniuse.com/mdn-javascript_classes_private_class_fields
| * @param delay the delay in milliseconds | ||
| * @returns {(function(...[*]): void)|*} a debounced version of the function | ||
| */ | ||
| #debounce(fn, delay) |
There was a problem hiding this comment.
I'm surprised we don't have an existing debounce method we can import from somewhere else.
| @@ -241,6 +241,8 @@ define('xwiki-requiredrights-dialog', [ | |||
| event.preventDefault(); | |||
| this.save(); | |||
| }); | |||
|
|
|||
| this.#setupResponsiveLayoutInitialization(); | |||
There was a problem hiding this comment.
I guess this method is called to perform something equivalent to the removed CSS, and to avoid having hardcoded width in the media queries, but it's not so clear to me by reading the code.
Also wdyt of explaining the technical constraints forcing us to do something we usually do in CSS?
There was a problem hiding this comment.
There is a comment below #setupResponsiveLayout that might explain what we do here. See also my comments in the PR description, I'm not 100% sure it's the best way to do it.
| * We use JavaScript to manage the responsive layout as we don't know in advance the width of the rights | ||
| * list in horizontal layout, and thus we can't define a breakpoint in CSS using a media/container query. | ||
| * This could be reconsidered when CSS environment variables become available which should greatly simplify | ||
| * this code. |
There was a problem hiding this comment.
Here is the comment that hopefully explains why we use this method.
| /** | ||
| * @returns {number} the minimum width of the rights list in horizontal layout | ||
| */ | ||
| #computeMinWidth() |
There was a problem hiding this comment.
The name of the method also does not state clearly that it has side effects on this.rightsList
There was a problem hiding this comment.
I agree with your conclusion that it's not satisfying to use JavaScript to handle this, but I fail to think of a better approach.
Maybe @tkrieck knows a way to change the grid system of an element based on an overflow?
In CSS, for now, there's nothing exactly like that. We could use flex positioning for larger viewports and switch to the proper mobile view in grid using a breakpoint ( Until the breakpoint is reached, the items in the list will wrap, like the image below. While I agree that is not as nice as the solution with JS, it's perfectly usable. 
|
Jira URL
https://jira.xwiki.org/browse/XWIKI-23332
Changes
Description
Clarifications
visibility: hiddenandheight: 0. This would actually solve the scenario described in the Jira issue even without any JavaScript code.I would be happy to get some feedback on these options that I mentioned above or also any other feedback and then I can update the code.
Screenshots & Video
Bildschirmaufnahme_20250624_151651.webm
Executed Tests
Manual tests as we have no real way to test such visual changes.
Expected merging strategy