-
Notifications
You must be signed in to change notification settings - Fork 80
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
XRENDERING-736: Macros info, success, warning and error are only distinguished by colors #286
Conversation
…uished by colors * Added the appropriate changes to the block structure of the message macro Note: Those icons use the Silk icon theme.
Would be great to see a screenshot of how it looks like when there is a title. |
Thanks Lucas.
Don't hesitate to ask Thiago for a review (and suggestions). |
} else { | ||
// Enhance the default box with an icon in the top left. | ||
Block defaultBox = boxFundation.get(0); | ||
Block iconBlock = new ImageBlock(this.getIconReference(), true); |
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.
An actual image inserted in the content does not feel like the cleanest. I would have expected the exact icon to be a CSS (so on platform side) decision based on the class (like the color).
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.
We want the icon to be accessible from a screen reader, so unfortunately we can't just add it in the :before
pseudo-element of the message box.
I thought the best practice would be to add an image in it, but I think I could also just add an empty Format Block
just like for the title, and somehow fill it with CSS.
However AFAIK, the various iconthemes also have various ways to represent icons in HTML, some use images, some use fonts, ... so I'm pretty sure we'd need more than CSS to solve this rendering completely.
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.
We have unfortunately no way to insert icons by CSS unless I missed something. Regarding the screen reader aspect, that's easy to solve I think by adding a sr-only
text. But then again, I'm not really sure that this is something we should do in rendering as sr-only
is not a standard but comes from Bootstrap iirc. Imho it would be cleanest to override these macros in xwiki-platform
to add proper icon HTML from the icon theme.
Setting this PR as a draft to add a block around the title, so that it's easier to style. |
…uished by colors * Added a container for the box title Note: These changes go beyond the context of message macros.
@@ -10,9 +10,9 @@ | |||
beginDocument | |||
beginMacroMarkerStandalone [custombox] [] [something] | |||
beginGroup [[class]=[box]] | |||
beginMetaData [[non-generated-content]=[java.util.List<org.xwiki.rendering.block.Block>][parameter-name]=[title]] |
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.
Does it mean that the title is not inplace editable anymore? AFAIR those metadata blocks were there on purpose to allow direct edition.
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.
Indeed, without this block, it's not possible to select the title for direct inplace editing. We can only change it through the message box properties.
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.
👍 Addressed in dcd57c2
Note that the presentation from xwiki-platform is broken when inplace editing too ^^'
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.
Shouldn't the title be a block element like <div>
? I remember that we had a problem that inline elements weren't editable in CKEditor. See https://jira.xwiki.org/browse/XWIKI-17417
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.
AFAICS this works properly with the changes proposed on xwiki-platform:
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.
Hm, yes, though our inline parsing is not really reliable so if you put something like == Big title ==
as title it will break WYSIWYG editing as the "corrected" HTML by the browser breaks the macro content markers.
…uished by colors * Added back the metadata block
.../xwiki-rendering-macro-box/src/main/java/org/xwiki/rendering/macro/box/AbstractBoxMacro.java
Outdated
Show resolved
Hide resolved
…stinguished by colors * Updated list instantiation Co-authored-by: Manuel Leduc <manuel.leduc@xwiki.com>
…uished by colors * Added an interface and a component to handle icon providing. Note: the default behavior is the same within xwiki-rendering, but there, xwiki-platform can override the component to allow usage of icon themes for transformations or generic icon use.
…uished by colors * Properly use the interface to instantiate the icons * Added a method to the interface to make sure the transformation icons work properly whatever the provider used * Fixed the POM
…uished by colors * Added a wrapping block for easier styling
Added some wrapping around the title + content of the block when necessary |
…uished by colors * Fixed the rule for wrapping children Note: Previous conditions were overcomplicated and did not generalize well (issue when using an image without a title)
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.
Thank you for updating this! I like the new approach with the icon provider. Just some details that need to be fixed. Also, please also open a rendering issue for this, or we should move the issue to rendering. It is fine to have a commit with a XRENDERING-Jira-issue in platform but not the other way around from my understanding.
The update of the icon transformation is nice, but I think it should be its own Jira issue and its own pull request or at least clearly separate commits as while I would backport the box macro to 15.10.x, I wouldn't do that for the icon transformation. The reason for this is that I'm worried that non-images might cause issues with exports. What happens if you export a document with smileys to an office format or LaTeX? I haven't tested this before so I don't know if icons were supported, but in general PNG icons should be much less of an issue than Font Awesome icons. Now I'm not saying that we need to fix this here, just something to think about in general and find a solution in XWiki 16.x.
@@ -10,9 +10,9 @@ | |||
beginDocument | |||
beginMacroMarkerStandalone [custombox] [] [something] | |||
beginGroup [[class]=[box]] | |||
beginMetaData [[non-generated-content]=[java.util.List<org.xwiki.rendering.block.Block>][parameter-name]=[title]] |
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.
Hm, yes, though our inline parsing is not really reliable so if you put something like == Big title ==
as title it will break WYSIWYG editing as the "corrected" HTML by the browser breaks the macro content markers.
import java.util.Set; | ||
import org.xwiki.component.annotation.Component; | ||
|
||
import javax.inject.Named; | ||
import javax.inject.Singleton; | ||
|
||
import org.xwiki.component.annotation.Component; | ||
import java.util.Set; |
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.
This reordering does not respect our code style, see https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HImports. Please configure your IDE to follow XWiki code style.
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.
Addressed in 95bae64 👍
* @param iconName the name of the icon needed | ||
* @return the block containing the icon | ||
*/ | ||
public Block get(String iconName) |
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.
Missing @Override
, also don't duplicate the Javadoc, see https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HDonotduplicateJavadoc
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.
Addressed in 95bae64 👍
* @version $Id$ | ||
* @since 15.10.2 | ||
*/ | ||
@Role |
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.
This needs to be @Unstable
.
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.
Addressed in 95bae64 👍
...-transformation-icon/src/main/java/org/xwiki/rendering/transformation/icon/IconProvider.java
Outdated
Show resolved
Hide resolved
// Add an image block as the last block | ||
pointer.addChild(new ImageBlock(new ResourceReference(iconName, ResourceType.ICON), true)); | ||
// Add an icon block as the last block | ||
pointer.addChild(iconProvider.get(iconName)); |
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.
The documentation mentions that the icon names are silk icon names. Are all silk icon names available in all icon themes? Also, I would have expected a separate Jira issue for this change.
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.
Are all silk icon names available in all icon themes?
No, but there's a fallback in the iconProvider that creates an ImageBlock like it used to be here. So even if it doesn't find the icon in the mapping, it'll put a Silk icon instead.
Also, I would have expected a separate Jira issue for this change.
This relies on the iconProvider class created in this PR, and it's a necessary change to solve XWIKI-21452 AFAIK, so I think it makes sense to leave it here.
@Singleton | ||
public class DefaultIconProvider implements IconProvider | ||
{ | ||
private static final List<Class> ICON_CLASS = new ArrayList<Class>(List.of(ImageBlock.class)); |
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 don't think this is needed at all, but still, why are you using an ArrayList
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.
Because the iconProvider can send back multiple kinds of icons. See https://github.com/xwiki/xwiki-platform/pull/2590/files#diff-8d2b165ed0bf133c701f7ce2d59119be83117a10967c9257eeb97990143ad7abR73
This one relies on the default behaviour and can send back RawBlocks and ImageBlocks.
This wouldn't be needed when implementing #286 (comment)
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA | ||
* 02110-1301 USA, or see the FSF site: http://www.fsf.org. | ||
*/ | ||
package org.xwiki.rendering.transformation.icon; |
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 I would put this in xwiki-rendering-api
. I don't know if this opinion is shared by others, but to me this icon transformation doesn't seem very modern, and it is causing many issues (like it breaks the WYSIWYG experience as it cannot be applied in WYSIWYG) which is why I would actually prefer to just remove it/turn it into a contrib extension.
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.
Addressed in cb8e83d 👍
@@ -41,5 +41,10 @@ | |||
<artifactId>xwiki-rendering-macro-box</artifactId> | |||
<version>${project.version}</version> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.xwiki.rendering</groupId> | |||
<artifactId>xwiki-rendering-transformation-icon</artifactId> |
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 don't think a macro as simple as the message macro should depend on the icon transformation. But see also my comment on the IconProvider
.
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.
Addressed when moving the IconProvider
to xwiki-rendering-api
👍
|
||
import javax.inject.Inject; | ||
import java.util.Collections; | ||
import java.util.List; |
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.
Wrong import order (see also below).
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.
Addressed in 95bae64 👍
…inguished by colors * Updated the logic for the IconTransformation parser to fit the new composite type
…inguished by colors * Fixed an outdated POM * Cleaned up code
…inguished by colors * Fixed import style
Opened XRENDERING-736: Macros info, success, warning and error are only distinguished by colors and updated the title of this PR :) Note: |
* @version $Id$ | ||
* @since 15.10.4 | ||
*/ | ||
public class IconBlock extends CompositeBlock |
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 didn't suggest introducing this as an API to be used outside the rendering transformation. Also, if you really want to make this an API, this needs to be @Unstable
. To me, having an IconBlock
would suggest that it somehow actually represents an icon, which is not the case here. To me, the use of this is too limited to justify having a new kind of block.
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 moved the IconBlock class in the IconProvider
. This way, it's easily accessible from any class that might need it.
I still don't know if this is a correct place but this looks less wrong to me.
See 8798113
From what I understand, it becomes an unstable API, but less open ended, we have a clear context in which to use it.
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.
Okay, I removed the IconBlock
class altogether, replacing it with a static parameter to give to the icon block, in da1ab07. This has the advantage of not changing the nesting for icon blocks and not breaking the tree traversal algo that was before.
The disadvantage:
- using this parameter name-value combination will break the tree traversal and the block will be interpreted like an icon.
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.
After seeing your comment at #286 (comment), I moved this parameter to the iconTransformation class and made it private.
I also updated the name of the parameter to decrease the risk of collision with another feature.
Changes in 5caf98e
* @param iconName the name of the icon needed | ||
* @return the block containing the icon | ||
*/ | ||
IconBlock get(String iconName); |
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 I would have let this return a Block
.
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.
This would open the door to implementing iconProviders that would break the icon transformation feature. I think it's for the best to rely on a more precise class with minimal expectations.
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 have the feeling you kind of misunderstood my original idea. My idea was that the icon transformation wraps whatever block the IconProvider
returns inside an IconBlock
, but an IconBlock
class that is private to the icon transformation. That way, the icon transformation knows also that this IconBlock
has exactly one child. This IconBlock
class would not be used anywhere else.
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.
Thank you for your help!
I think I understand it now :)
See #286 (comment)
for (int i = 0; i < count - 1; i++) { | ||
matchStartBlock.getParent().removeBlock(matchStartBlock.getNextSibling()); | ||
} | ||
sourceBlock = mappingCursor.clone(); | ||
sourceBlock = mappingCursor | ||
.getFirstBlock(AnyBlockMatcher.ANYBLOCKMATCHER, Block.Axes.DESCENDANT).clone(); |
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 are you cloning the first child of the icon block? What if the icon block has several children?
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.
The way I defined and used the IconBlock so far is that it contains only one child which is a block containing the whole icon.
After thinking about it, there's no reason to exclude other use cases where multiple blocks are given as children, possibly imposing an extra wrapping that's not needed.
After looking at the way we parse trees, it's not straigthforward to allow multiple children.
I have trouble understanding what this function does, there are a lot of abstract concepts and minimal documentation (and the recursive nature of the function does not make things simpler). With a simple test (1. make sure the default renderer behaves correctly, and 2. make sure a renderer that add a random wordBlock behaves as expected) I couldn't find out an easy change that would return the expected value for tests on both cases...
I think it's okay to assume that all the content in the Icon block is nested inside a singular block.
I'm adding a line in the definition of the IconBlock to highlight this.
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.
Wrapping the children blocks in a composite block when they are multiple does not work either.
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 don't you just clone the IconBlock
itself, i.e., just revert the change on this line?
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.
Somehow it fails at transforming multiple icons one after the other:
https://up1.xwikisas.com/#6lC3HfY35P2Qv2BVGf6LKw
…inguished by colors * Moved around the IconBlock class
…inguished by colors * Replaced the IconBlock class with a parameter
…inguished by colors * Moved the parameter from the IconProvider interface to the private usage of IconTransformation
…inguished by colors * Moved the IconProvider classes to util instead of transformation
Changes since #286 (comment) :
Executed testsAs of the end of af1759b, I could successfully build:
which covers all the changes. |
…inguished by colors * Use a custom block to wrap icons in the icon transformation instead of using a parameter that would be present in the output.
Thanks for your help Michael! |
As mentioned in the chat, the changes to the icon transformation should be separated from the changes for XRENDERING-736 as they are completely independent, they are just much easier now. The potential problem I see with this change is that export to different formats like office formats or LaTeX won't handle HTML output and thus won't display the icons anymore which could be seen as a regression. I think we need to re-think how we handle icons but regardless how we handle icons in the future, it should be possible to adjust the icon provider to the chosen solution. |
…inguished by colors * Reverted all changes on the IconTransformation Note: adding them in a comment for XRENDERING-738 so that we can use them later if needed
I removed all the changes to the icon transformation in a451d60. I created a ticket for this issue: https://jira.xwiki.org/browse/XRENDERING-738 and added the fix discussed here as a comment. Additionally, after a last check, all the builds mentionned above still pass sucessfully. |
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.
Thank you, just a few minor suggestions for documentation update.
I fear, following the comment of @vmassol on #2775 there should be a forum proposal that details the UI change with the exact new look that was chosen before this can be merged.
xwiki-rendering-api/src/main/java/org/xwiki/rendering/util/IconProvider.java
Outdated
Show resolved
Hide resolved
xwiki-rendering-api/src/main/java/org/xwiki/rendering/util/IconProvider.java
Outdated
Show resolved
Hide resolved
xwiki-rendering-api/src/main/java/org/xwiki/rendering/internal/util/DefaultIconProvider.java
Outdated
Show resolved
Hide resolved
I started a forum proposal on https://forum.xwiki.org/t/updating-the-presentation-of-message-macros/13872 👍 |
…inguished by colors * Updated `since` fields Co-authored-by: Michael Hamann <michael@content-space.de>
…inguished by colors * Updated `since` fields Co-authored-by: Michael Hamann <michael@content-space.de>
…inguished by colors * Improved javadoc Co-authored-by: Michael Hamann <michael@content-space.de>
xwiki-rendering-api/src/main/java/org/xwiki/rendering/internal/util/DefaultIconProvider.java
Outdated
Show resolved
Hide resolved
…inguished by colors * Rewriting of a selector using a more straightforward syntax. Note: It has been built with standard tests successfully and behaves the same as before for manual tests on a local distribution.
…inguished by colors * Updated `since` annotations
Jira
https://jira.xwiki.org/browse/XRENDERING-736
PR Changes
Note
When testing on a default XWiki distribution, those icons use the Silk icon theme despite the selected color theme being Font awesome 4.
Tests
Passed successfully
mvn clean install -f xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-message/
.Here, we can see that the icons are properly added on a sandbox page with message boxes of the four types.
However, additional style must be provided for them to look better, which will be done in a pull request for
xwiki-platform
.EDIT: Updated Jira URL
Also see xwiki/xwiki-platform#2590 on
xwiki-platform