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-21778: Admin section: make the Extension section pass webstandard tests #2812

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Jan 22, 2024

Jira URL

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

Changes

Description

  • Replaced inline styling with a use of the meter element
  • Added the webstandard test for the extension section.

Clarifications

  • The meter style options are not standard HTML yet, so multiple style need to be used for webkit based browsers and Firefox. Changes have been tested manually with Firefox 121 and Chrome 120
  • Regarding accessibility, it seems like the doc about meter on the Aria Practice Guide is not up to date

Screenshots & Video

No UI Change expected.
before vvv
21778-beforePR
after (Firefox) vvv
21778-afterPRfirefox
after (Chrome) vvv
21778-afterPRChrome

Executed Tests

Successfully passed mvn clean install -f xwiki-platform-distribution/xwiki-platform-distribution-flavor/xwiki-platform-distribution-flavor-test/xwiki-platform-distribution-flavor-test-webstandards -Dxwiki.test.startXWiki=false for the added page /xwiki/bin/admin/XWiki/XWikiPreferences?editor=globaladmin&section=XWiki.Extensions.
Manual tests for Firefox 121 and Chrome 120. Browser support is pretty approximative, it might be necessary to check it on Safari and Edge too, even though it should be okay (based on Webkit like Chrome).

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • N/A

…rd tests

* Replaced inline styling with a use of the meter element

Note: the meter style options are not standard HTML yet, so multiple style need to be used for webkit based browsers and Firefox. Changes have been tested manually with Firefox 121 and Chrome 120
…rd tests

* Added the webstandard test for the extension section.
<li class="current-rating" style="width:${width}%;"></li>
<li class="current-rating">
<meter class="average-rating" min="0" max="5" value="$rating"/>
</li>
Copy link
Member

Choose a reason for hiding this comment

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

Very interesting case of "is this a potentially breaking DOM change or not?" :) I mean: here the chance that anyone is impacted is absurdly low so I don't see any reason to not accept the changes. But, in theory we could imagine somehow relying on some code to check the width of a current-rating class in javascript to perform some stuff...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should set Admin UI as an API.
From what I can remember, this rating component is custom to the extension admin section.


(paste of an answer provided on live chat)
The style attribute in the DOM is the one JS really shouldn't rely on to work x)
It's been a while that we have don't use inline style declarations in our front end code style

Copy link
Member

Choose a reason for hiding this comment

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

So as @Sereza7 mentioned in the chat:

It's been a while that we have don't use inline style declarations in our front end code style

so then it's clearly not an API we're breaking :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(more discussion from the chat, moved here for the sake of keeping long term traces)

I think this rating_macros.vm is also used to display ratings in pages, so it's not only for the admin
it's just that you don't have the Ratings Extension bundled by default so you need to install it first: you might want to check that

I just finished checking simple interactions, and it seems to behave like the one in extension behaved. I made minimal modifications, only removing the default inline styling and add a meter node in the list item. The part that would be difficult to handle for backward compatibility (handling how we update this at runtime) is not changed. It's not perfect, but the goal of this ticket is to avoid breaking web standard tests (only run at page load) and backward compatibility.

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

Successfully merging this pull request may close these issues.

2 participants