Skip to content
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

XWIKI-22485: Livedata option dropdown has no grouping semantics #3662

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Nov 20, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22485

Changes

Description

  • Updated the structure of the LiveData dropdown (namely, removed separator
  • s and grouped links together)
  • Updated bootstrap dropdown styles to account for this new kind of separator in dropdowns.
  • Updated the tests to work on this updated architecture

Clarifications

  • Using separator list items can cause weird experiences for AT users, so instead of this I decided to just nest sublists inside the main list to make a semantic separation between the items.
  • I brought the style changes directly on our fork of bootstrap because it made the most sense here. I could have added this style in the liveData code, but this change is generic so I thought it would be best as an update of our design system.
  • I decided to keep the changes on the tests minimal. I could have reworked them to make them simpler. The workaround that was used is no longer needed. However, in order to simplify this, I'd need to rewrite a large part of each test because of the way it was coded. I went with the easy solution, feel free to let me know if we should add more changes to the tests in this PR.

Screenshots & Video

On the left is the dropdown before the changes in this PR, and on the right is the same dropdown after the changes in this PR. We can see that the styles are pretty much the same. This was the goal of this PR: improve the HTML architecture of the dropdown without regressing on the style.
Screenshot from 2024-11-20 12-05-23

Executed Tests

Successfully built the livedata webjar with the quality profile: mvn clean install -f xwiki-platform-core/xwiki-platform-livedata/xwiki-platform-livedata-test/xwiki-platform-livedata-test-docker/.
Successfully passed livedata docker tests: mvn clean install -f xwiki-platform-core/xwiki-platform-livedata/xwiki-platform-livedata-test/xwiki-platform-livedata-test-docker/.
Manual tests on a local instance to check for style with Firefox (see screenshot above).
Sonarlint analysis on the updated files did not point out any recent quality regressions.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 16.10.X

* Updated the structure of the LiveData dropdown (namely, removed separator <li>s and grouped links together)
* Updated bootstrap dropdown styles to account for this new kind of separator in dropdowns.
* Updated the tests to work on this updated architecture
@@ -47,7 +47,7 @@
<ul class="dropdown-menu dropdown-menu-right">

<!-- Actions -->
<li class="dropdown-header">{{ $t('livedata.dropdownMenu.actions') }}</li>
<li><span class="dropdown-header">{{ $t('livedata.dropdownMenu.actions') }}</span><ul>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of indenting the new ul block?

Copy link
Contributor Author

@Sereza7 Sereza7 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 53d85de 👍
I indented only once for the <li><ul> blocks though.

In order to avoid making code review more difficult for barely any reason, I try to not add indentation at first on code blocks where it looks okayish without it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead with the right formatting.

<li role="separator" class="divider"></li>

<li class="dropdown-header">{{ $t('livedata.dropdownMenu.layouts') }}</li>
<li><span class="dropdown-header">{{ $t('livedata.dropdownMenu.layouts') }}</span><ul>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@Sereza7 Sereza7 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 53d85de 👍
Note that I updated all three uls

Copy link
Contributor

@manuelleduc manuelleduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Remains one code indentation remark.
You don't mention what was wrong/what is improved for users using assistive technologies, it could be useful to understand what you try to achieve in this PR.

@Sereza7
Copy link
Contributor Author

Sereza7 commented Nov 27, 2024

You don't mention what was wrong/what is improved for users using assistive technologies, it could be useful to understand what you try to achieve in this PR.

I'll add this paragraph directly as a comment on the ticket too for reference sake.
As a user with vision, when you see this dropdown
image
You understand that there's a list of options/ actions, and that this list is split into subcategories. This is the semantics we share through style.
The semantics we share through structure of the HTML should be similar. Before this PR, it was:
image
A list of options/ actions. We did not convey any information about the groups in the HTML.

For the sake of users of assistive tools, and overall accessibility of XWiki, you want to make sure the semantics you convey through your HTML are roughly the same as the semantics you convey through your style. This is adaptability.

This PR makes sure we represent those subcategories in the HTML structure. Additionally, it removes the "I am one item of the list" semantics from the separator, because a separator is not an item anyone would name when asked to list the elements of this dropdown (meaning it has no semantics and should be showed using non semantic elements/ no element style only).


Before AT would approx read:
"List with 6 items - item 1:Actions - item 2: Refresh - item 3:separator - item 4: Layouts - item 5:Table - item6: Cards [...]"
With the new structure they would read:
"List with 2 items - item 1: Actions, List with 1 item - item 1: Refresh - item 2: Layouts, List with 2 items - item 1: Table - item 2: Cards"

* Improved indentation in the dropdown menu template
{{ panel.name }}
</a>
</li>
<li><span class="dropdown-header">{{ $t('livedata.dropdownMenu.panels') }}</span><ul>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that part could be indented as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants