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.

Sign up for GitHub

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

Jump to bottom

XWIKI-21633: Adding a step tour on a class field doesn't work #3781

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

XWIKI-21633: Adding a step tour on a class field doesn't work #3781

wants to merge 2 commits into from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Dec 27, 2024 •
edited
Loading

Jira URL

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

Changes

Description

  • Removed XWiki interpretation from the JSON construction velocity template in TourJson
  • Added a hint for the element field of the step creation/edition forms.
  • Added a javascript log when the retrieval of the tour's JSON fails
  • Added the english value for the element field hint.

Clarifications

  • The main change is the one line in the TourJSon file.
    • We need one backslash to escape a special character in CSS. \
    • We escape this backslash when serializing the tour data to a JSON \\
    • XWiki interpreter figures out it's the newline syntax and replaces it with \n
    • The first two steps are correct, but the third one is unexpected. Since this is a standard behaviour of the XWiki syntax interpreter, the solution is to avoid using it here. The line in the TourJSON file fixes this.
  • The other changes in this PR are just small quality improvements:
    • Addition of a hint to the element field so that it's easier to use by admins
    • Addition of a failure message when retrieving the JSON for easier debugging (without it, this kind of tour failure was completely silent)

Screenshots & Video

Both of those screenshots are taken after applying the changes proposed in this PR:
Screenshot from 2024-12-27 15-58-24
Screenshot from 2024-12-27 15-58-54

Executed Tests

Successfully passed mvn clean install -f xwiki-platform-core/xwiki-platform-tour/ -Pquality,integration-tests,docker -Dxwiki.test.ui.wcag=true with no WCAG warning.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 16.10.X (localized changes, relatively safe)

@Sereza7
XWIKI-21633: Adding a step tour on a class field doesn't work
89753ae 
* Added a hint for the element field of the step creation/edition forms.
* Added a javascript log when the retrieval of the tour's JSON fails
* Removed XWiki interpretation from the JSON construction velocity template in TourJson
* Added the english value for the `element` field hint.
@Sereza7 Sereza7 requested a review from surli December 27, 2024 15:27
surli
surli reviewed Jan 7, 2025
View reviewed changes
...tform-core/xwiki-platform-tour/xwiki-platform-tour-ui/src/main/resources/TourCode/TourJS.xml Outdated
@@ -428,7 +428,10 @@ require(['jquery', 'xwiki-meta'], function ($, xm) {
createTour(tour);
}
}
}).fail(function (data) {
console.log("Querying the JSON for the Tour failed. %o", data);
Copy link
Member

Choose a reason for hiding this comment

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

should be an error log instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d8aade5 👍

surli
surli reviewed Jan 7, 2025
View reviewed changes
.../xwiki-platform-tour/xwiki-platform-tour-ui/src/main/resources/TourCode/TourTranslations.xml Outdated
@@ -69,6 +69,8 @@ tour.popover.show.hint=You can restart the tour by clicking this button at anyti
# Steps
TourCode.StepClass_order=Order
TourCode.StepClass_element=Element (CSS selector that identify an element)
TourCode.StepClass_element.hint=CSS selector that identifies an element. Escape special CSS characters with a
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d8aade5 👍

@surli
Copy link
Member

surli commented Jan 7, 2025

You said:

The main change is the one line in the TourJSon file.
We need one backslash to escape a special character in CSS.
We escape this backslash when serializing the tour data to a JSON \
XWiki interpreter figures out it's the newline syntax and replaces it with \n

It's not entirely clear to me: you didn't actually performed any changes to add the backslashes. I assume you mean those still need to be put manually but the only change is on step 4: now those are not transformed anymore to \n? is that correct? So if I understand properly your changes don't fix using Code.TestClass_input but it fixes using Code\.TestClass_input, right?

@Sereza7
XWIKI-21633: Adding a step tour on a class field doesn't work
d8aade5 
* Fixed code style.
* Updated the JS log type.
@Sereza7
Copy link
Contributor Author

Sereza7 commented Jan 24, 2025

You said:

The main change is the one line in the TourJSon file.
We need one backslash to escape a special character in CSS.
We escape this backslash when serializing the tour data to a JSON \
XWiki interpreter figures out it's the newline syntax and replaces it with \n

It's not entirely clear to me: you didn't actually performed any changes to add the backslashes. I assume you mean those still need to be put manually but the only change is on step 4: now those are not transformed anymore to \n? is that correct? So if I understand properly your changes don't fix using Code.TestClass_input but it fixes using Code.TestClass_input, right?

Note that when you read the CSS selector, both should have a different meaning:

  • #Code.TestClass_input is the object with the Code id and the TestClass_input class
  • #Code\.TestClass_input is the object with the Code.TestClass_input id

Since by default, ids are not supposed to contain points, we want to make sure we don't change anything for the first one (it could be seen as a regression for a lot of existing Tours). The second one is non standard (even if common, in HTML ids are not supposed to have dots in them) and we had an unexpected behaviour with it. Removing the XWiki syntax rendering on this one allows for it to work as expected.

I updated the first message clarifications to hopefully be a bit better :)

@Sereza7 Sereza7 requested a review from surli January 24, 2025 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surli surli Awaiting requested review from surli

Assignees

@surli surli

Labels
backport stable-16.10.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sereza7 @surli