-
-
Notifications
You must be signed in to change notification settings - Fork 552
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-21878: Various close button modals do not use intended icons #2888
Conversation
* Removed the hard coded values from the templates. * Made a REST API call from modal.js to access the current icontheme icon. Note: The only occurrence of `×` left in the project is the fallback value for the modal.js template.
var modalTemplate = | ||
// Fetch the cross icon from the icon theme to fill up the modal template. | ||
let iconURL = `${XWiki.contextPath}/rest/wikis/${XWiki.currentWiki}/iconThemes/icons?name=cross`; | ||
let iconRequest = new XMLHttpRequest(); |
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.
any reason to not use a jquery $.get
call here?
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.
No specific reason. I don't have much experience with async javascript and API calls yet. I'll spend some time to try and use jquery instead 👍
Setting this PR as a draft until this is done
* Tried to change modal API call syntax
* <WIP fixing the async call to API>
* Finished changing the library used to call the REST API
* Improved codestyle
Used jquery |
'<button type="button" class="btn btn-primary" disabled="disabled">OK</button>' + | ||
'</div>' + | ||
// Fetch the cross icon from the icon theme to fill up the modal template. | ||
let iconURL = `${XWiki.contextPath}/rest/wikis/${XWiki.currentWiki}/iconThemes/icons?name=cross`; |
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.
I think the wiki name should be URL-encoded to protect against wiki names with special characters (not sure if we allow them, though).
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.
https://github.com/search?q=repo%3Axwiki%2Fxwiki-platform%20%24%7BXWiki.currentWiki%7D&type=code
It's already used a few times in the codebase.
I took the opportunity to fix those other uses in 4 files in 217df8a .
I rebuilt the ckeditor plugin with the change, and it didn't break anything when testing on the main wiki :)
...wiki-platform-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-dialog/modal.js
Show resolved
Hide resolved
* URIEncoded current wiki names
Jira URL
https://jira.xwiki.org/browse/XWIKI-21878
Changes
Description
Clarifications
×
left in the project is the fallback value for the modal.js template.Screenshots & Video
All of those screenshots are taken after the changes in this PR. Hereafter are four of those. Among them, there's the "more difficult' cases with icons integrated in javascript (
modal.js
andsyntaxPicker.js
).Executed Tests
Manual tests with FA4, see screenshots. I also checked the modal.js template behaved as expected with Silk, and it behaved as expected.
No test build conducted. From what I could see, this change does not reflect into any test (except the resources
xwiki-valid.html
andxwiki-invalid.html
). Passes regular builds:mvn clean install -f xwiki-platform-core/xwiki-platform-ckeditor/xwiki-platform-ckeditor-plugins; mvn clean install -f xwiki-platform-core/xwiki-platform-ckeditor/xwiki-platform-ckeditor-webjar/
needed for testing on a live instance.Expected merging strategy