-
-
Notifications
You must be signed in to change notification settings - Fork 555
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-22433: Provide consistency in info / warning / error class usages in templates #3555
base: master
Are you sure you want to change the base?
Changes from all commits
07f39f0
d686447
358f386
66d842d
049209d
82afaa7
66b5761
93a4ccf
8757b5d
fd3109f
98a16a1
c96ff86
72bbd40
47b7d7c
3a0b6bc
3e07ab8
49c9494
dae36ae
3884bde
d9f7855
6def82e
a5f8f97
1c7764b
016dc7a
422b1c4
734ab84
a0e4fdc
6452b5c
950cbdb
35814cc
6fbafbd
07dfee6
2fc20cf
6b9cce1
87fe4a4
f75a44c
92fab0b
4b60a92
3e3ddb8
8d2d174
161d3d0
20b86ef
c131356
7a3e02c
b5394a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -244,7 +244,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#macro (maybeShowDeleteUserWarning $userReference $right) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#countPagesLastModifiedBy($userReference) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#if ($pageCount > 0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<div class="box errormessage xform"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{{/html}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{{error cssClass="xform"}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{{html clean="false"}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm and why do we need the clean false here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right before starting the velocity macro, we closed the "current" html macro. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
that's a safest bet to avoid regression without performing a check, but I'm not sure it's a good solution or a good reflex to have: IMO using clean=false attribute in html macro should only be used for specific identified cases. It's quite possible I'm wrong, but I don't really see what we couldn't use clean true here. IMO it worth using clean true by default, and then checking manually the behaviour. Longer to test, for sure, but better for long term maintenance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that another potential reason to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm I didn't thought about that. Now I guess better be safe than fast in this specific condition? wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I don't have a strong opinion on either side. I guess it depends on how complex the content is and how often it's used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#set ($pageIndexReference = $services.model.createDocumentReference( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
$userReference.wikiReference.name, 'Main', 'AllDocs')) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#set ($pageIndexURL = $xwiki.getURL($pageIndexReference) + '#|t=alldocs&doc.author=' + | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -273,7 +276,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[$rightTranslation]))</span> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</dd> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</dl> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{{/html}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{{/error}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{{html clean="false"}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,13 +116,13 @@ | |
|
||
#if ($recommended) | ||
#if (!$extensions.iterator().hasNext()) | ||
<div class="box warningmessage">$services.localization.render('extensions.search.recommended.fallback', ["<em>$!escapetool.xml($request.search)</em>"])</div> | ||
#warning($services.localization.render('extensions.search.recommended.fallback', ["<em>$!escapetool.xml($request.search)</em>"])) | ||
## Search again with the recommended filter | ||
#set ($extensions = $repository.search("$!request.search", $paginationParams.firstItem, $paginationParams.itemsPerPage)) | ||
#set ($totalHits = $extensions.totalHits) | ||
#set ($recommended = false) | ||
#elseif (!$customExtensionFilter) | ||
<div class="box infomessage"> | ||
#define ($infoHeaderContent) | ||
#if ($compatible) | ||
$services.localization.render('extensions.search.compatiblerecommended.disclaimer') | ||
#else | ||
|
@@ -142,12 +142,13 @@ | |
<input type="submit" value="${escapetool.xml($services.localization.render('extensions.search.all.label'))}" class="btn btn-default"/> | ||
#end | ||
</form> | ||
</div> | ||
#end | ||
#info("$infoHeaderContent") | ||
#end | ||
#end | ||
|
||
#if (!$extensions.iterator().hasNext()) | ||
<div class="box infomessage">$services.localization.render($noResultsMessageKey, ["<em>$!escapetool.xml($request.search)</em>"])</div> | ||
#info($services.localization.render($noResultsMessageKey, ["<em>$!escapetool.xml($request.search)</em>"])) | ||
#else | ||
#if ($totalHits && $totalHits > $paginationParams.itemsPerPage) | ||
#set ($hasPagination = true) | ||
|
@@ -165,39 +166,46 @@ | |
#end | ||
|
||
#if ($indexed) | ||
#define ($formContent) | ||
<form action="${xwiki.relativeRequestURL}"> | ||
#if ($request.section) | ||
<input type="hidden" name="section" value="${escapetool.xml($request.section)}" /> | ||
#end | ||
<input type="hidden" name="search" value="$!{escapetool.xml($request.search)}" /> | ||
<input type="hidden" name="recommended" value="$recommended" /> | ||
<input type="hidden" name="indexed" value="$indexed" /> | ||
<input type="hidden" name="compatible" value="$compatible" /> | ||
#if ($indexJobStatus.state != 'RUNNING') | ||
<input type="submit" value="${escapetool.xml($services.localization.render('extensions.search.indexed.reindex'))}" name="index_start" class="btn btn-default"/> | ||
#end | ||
</form> | ||
#end | ||
#set ($indexJobStatus = $repository.getStatus("wiki:${xcontext.database}")) | ||
#if ($indexJobStatus) | ||
<div class="box infomessage"> | ||
#if ($indexJobStatus.state != 'FINISHED') | ||
$escapetool.xml($services.localization.render('extensions.search.indexed.started', [$xwiki.formatDate($indexJobStatus.startDate)])) | ||
#set ($discard = $xwiki.jsfx.use('uicomponents/job/job.js')) | ||
#set ($jobStatusURL = $doc.getURL('get', $escapetool.url({ | ||
'xpage': 'job_status_json', | ||
'outputSyntax': 'plain', | ||
'jobId': $indexJobStatus.request.id | ||
}))) | ||
<div class="xcontent job-status" data-url="$escapetool.xml($jobStatusURL)"> | ||
#displayJobProgressBar($indexJobStatus, true) | ||
</div> | ||
#define ($infoFooterContent) | ||
$escapetool.xml($services.localization.render('extensions.search.indexed.started', [$xwiki.formatDate($indexJobStatus.startDate)])) | ||
<div class="xcontent job-status" data-url="$escapetool.xml($jobStatusURL)"> | ||
#displayJobProgressBar($indexJobStatus, true) | ||
</div> | ||
#end | ||
#else | ||
$escapetool.xml($services.localization.render('extensions.search.indexed.on', [$xwiki.formatDate($indexJobStatus.startDate)])) | ||
#define ($infoFooterContent) | ||
$escapetool.xml($services.localization.render('extensions.search.indexed.on', [$xwiki.formatDate($indexJobStatus.startDate)])) | ||
#end | ||
#end | ||
#info("$infoFooterContent | ||
$formContent") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm you should really add a comment if the carriage return is on purpose here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just checked it again and it doesn't seem to me like this is needed. I just figured out it wouldn't hurt since it's HTML syntax, and it makes it a bit more readable. |
||
#else | ||
<div class="box warningmessage">$escapetool.xml($services.localization.render('extensions.search.indexed.nojob')) | ||
#warning("$escapetool.xml($services.localization.render('extensions.search.indexed.nojob')) | ||
$formContent") | ||
#end | ||
<form action="${xwiki.relativeRequestURL}"> | ||
#if ($request.section) | ||
<input type="hidden" name="section" value="${escapetool.xml($request.section)}" /> | ||
#end | ||
<input type="hidden" name="search" value="$!{escapetool.xml($request.search)}" /> | ||
<input type="hidden" name="recommended" value="$recommended" /> | ||
<input type="hidden" name="indexed" value="$indexed" /> | ||
<input type="hidden" name="compatible" value="$compatible" /> | ||
#if ($indexJobStatus.state != 'RUNNING') | ||
<input type="submit" value="${escapetool.xml($services.localization.render('extensions.search.indexed.reindex'))}" name="index_start" class="btn btn-default"/> | ||
#end | ||
</form> | ||
</div> | ||
#end | ||
#end | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,13 +43,11 @@ | |
#set($finished = $jobStatus.state.name() == 'FINISHED') | ||
#if ($finished) | ||
#if (!$jobStatus.logTail.hasLogLevel('ERROR')) | ||
<div class="box successmessage"> | ||
Done. | ||
</div> | ||
## Remove when the following issue is resolved: | ||
## TODO XWIKI-22710: Add translations for FilterStreamJobJSON | ||
#success('Done.') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're missing translations in that file it seems. Might worth adding a comment / creating ticket if there's none. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 87fe4a4 👍 |
||
#else | ||
<div class="box errormessage"> | ||
Error has been found during the conversion ! | ||
</div> | ||
#error('Error has been found during the conversion !') | ||
#end | ||
#end | ||
#end | ||
|
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.
Why do we need this xform here? for style only? My understanding is that the goal of the ticket was to simplify the usages of errormessage, so it feels a bit too complex than it should 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.
Yup, I want to minimize the changes in the actual content produced in order to minimize breaking customization and other features. I forgot the exact context here, but I don't want to break the form (style and functionality) just because I removed a class. Having this class is supported by the XWiki macro, so I figured it was okay to refactore things like that.
The goal of the PR is to factorize their code. This way, next time we update the message boxes, there won't be inconsistencies popping up everywhere (and this also fixes inconsistencies that popped up since we added icons with XWIKI-21452 :) ).
$${\color{orange}ONGOING}$$
In order to do so, in all of the scripts here, I either use the new velocimacro, or the XWiki macro if possible.