XWIKI-21452: Macros info, success, warning and error are only distinguished by colors #2590

Sereza7 · 2023-11-08T17:18:37Z

Jira

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

PR Changes

Updated style to take into account the addition of an icon in xwiki-rendering

Tests

No specific test since this is just style changes

View

Video demo to check out responsivisity of these style changes

Note

This PR should be merged on main, only after the appropriate changes have been merged on xwiki-rendering -> xwiki/xwiki-rendering#286 .

…uished by colors * Updated style to take into account the addition of an icon in xwiki-rendering

vmassol · 2023-11-09T09:11:40Z

Thanks for the updated screenshot. The separation between the title and the text is not very clear. I think we already have a jira issue about this (for the box macro since this is what is used underneath). I know this not your topic though and not really related to accessibility ;)

tmortagne · 2023-11-09T09:23:53Z

Yes, the styling of the title itself is the subject of another issue, I mainly wanted to see the placement of the icon compared to the title. Thanks, @Sereza7.

…uished by colors * Updated style to differentiate the title from the content of the box

Sereza7 · 2023-11-09T09:29:07Z

I can add a couple CSS rules to fix that in this PR ^^
12d5de0 sets a font-weight: bold on the title. This isn't much but I think it's enough:

This is a screenshot displaying the changes in the last commit, we can see that the title font is bolder, which differentiates it slightly from the content of the box.

tkrieck · 2023-11-09T10:02:20Z

Hi, I've done some mockups to test a better separation between elements. I don't know yet the difference between the small version and the big one on your screenshots, but I called them big and compact :) Some callouts have titles, others don't, just to check different versions.

I think it's important to maintain the horizontal padding consistent, so we can have better alignment between the two versions, as we can see below. Also, from the screenshot you can see that maintaining the text in its own box can help improve legibility as the beginning of the text line always start at the same vertical point.

If we could use Font Awesome it would be... well... awesome. The Silk iconset is pretty dated and, being a PNG, it does not render very well on a Hi Dpi monitor;

tmortagne · 2023-11-09T11:03:23Z

the difference between the small version and the big one

It's "inline" (when the macro is in the middle of a paragraph, for example) and "standalone" (completely isolated).

Sereza7 · 2023-11-09T11:18:44Z

@tkrieck

I don't know yet the difference between the small version and the big one on your screenshots, but I called them big and compact

Big -> Regular macro, box with a message inside. You need to make sure you have an empty line before and after the macro.
Compact -> Inline mode for the macro, span with the message inside (and no title). This one is used if you didn't leave an empty line before/after the macro. We can't really play too much with the padding here since we want it to not increase line height.

For reference here is the xwiki 2.1 syntax I used to generate the page in my screenshot.

I think it's important to maintain the horizontal padding consistent, so we can have better alignment between the two versions, as we can see below. Also, from the screenshot you can see that maintaining the text in its own box can help improve legibility as the beginning of the text line always start at the same vertical point.

How did you manage to get that style with the current HTML? I'm only seeing more and more style hacks we need to perform because there's a lack of container for the title. I'll add a container and update this PR soon so the style implementation can be more straightforward/stable.

If we could use Font Awesome it would be... well... awesome. The Silk iconset is pretty dated and, being a PNG, it does not render very well on a Hi Dpi monitor;

Unfortunately in this context we can't use font-awesome without a lot more work (see xwiki/xwiki-rendering#286 ). It's actually not that bad here because the icons have approximately the same colors as the blocks. IMO it's way worst with transformation icons for example.

Setting this PR as a draft until I add a container for the title, set up vertical alignment and update padding :)

tkrieck · 2023-11-09T14:10:46Z

How did you manage to get that style with the current HTML?

Those are only mockups, but the alignment should be doable with an absolute positioning of the icon inside a relative positioned container for the message or even better with a flex layout;

Regarding the title, yeah, a container would help to achieve this effect without hacks on the CSS.

Unfortunately in this context we can't use font-awesome without a lot more work

No problem.

Edit: I forgot to address your first comment

We can't really play too much with the padding here since we want it to not increase line height.

No problem, disregard my compact version then

michitux · 2023-11-09T17:12:29Z

I think it's important to maintain the horizontal padding consistent, so we can have better alignment between the two versions, as we can see below. Also, from the screenshot you can see that maintaining the text in its own box can help improve legibility as the beginning of the text line always start at the same vertical point.

What about long messages without title? Would you indent them the same way as with title?

tkrieck · 2023-11-09T17:23:53Z

What about long messages without title? Would you indent them the same way as with title?

If they have icons before them, then yes.

I don't know if there will be support from now on for messages without the icons, but if that's the case, then the text aligns normally with the message box, as it is today;

Example expanded with more options

…uished by colors * Updated style to align text in the box with a grid. Note: we don't know the exact height we want for this box (depends on content), so we can't use a column flexbox.

…uished by colors * Cleaned up style

…uished by colors * Updated padding and icon position for a cleaner presentation

…uished by colors * Fixed width of the boxes fitted to content and icon alignement

Sereza7 · 2023-11-10T15:52:40Z

PR Changes

Added style to fit the new box-title class
Replaced the flexbox display with a grid to support vertical alignment of title and content.

Note

We don't know the exact height we want for this box (depends on content), so we can't use a column flexbox.

View

…uished by colors * Fixed presentation for inplace editing

Sereza7 · 2023-11-15T10:22:27Z

Fixed the presentation for inplace editing in d6ed654 :

Inplace editing wraps the nodes we expected to be in the message box in a layer of CKEditor nodes. I added a selector to apply the same style to those CKEditor nodes as the one applied on the base nodes.

michitux · 2023-12-07T15:24:30Z

PR Changes
* Added style to fit the new `box-title` class

* Replaced the flexbox display with a grid to support vertical alignment of title and content.
Note

We don't know the exact height we want for this box (depends on content), so we can't use a column flexbox.

View

This doesn't follow the styles proposed by @tkrieck - is there any reason for it? In particular, the icons have way less spacing to the border and are not on the same line as the text when there is no title. This looks very strange in my opinion. What @tkrieck looks much better in my opinion.

Also, I'm really not happy with introducing silk icons here. Even if we replace these icons by SVG icons from Font Awesome, this won't look good as they won't have the text color. I'm really wondering if it wouldn't be better to either add better icon support to xwiki-rendering or to override these macros in xwiki-platform. I think we should first discuss how to continue with icons before merging this.

Sereza7 · 2023-12-08T16:52:11Z

In particular, the icons have way less spacing to the border

Here is a side by side comparison of the two for easier assessment of the difference

I didn't think the value of the border spacing was one of the main point of the proposal made by @tkrieck so I tweaked it to avoid reducing too much the space available for content.

Here is an example much closer to the styles proposed by tkrieck. In my opinion, especially on multi line blocks, having such a large column just to fit an icon that is non essential to understanding the content for most users looks overkill.

not on the same line as the text when there is no title.

In my opinion, the icon floating in the middle of the left column didn't look pretty on large blocks of text, mostly because sometimes it didn't line up with the line next to it. So I decided to go with the choice to keep it consistently in the same place whether or not there's a title for the block. I think it's good because it emphasizes that the icon is related to the block itself and not its content.

Also, I'm really not happy with introducing silk icons here. Even if we replace these icons by SVG icons from Font Awesome, this won't look good as they won't have the text color. I'm really wondering if it wouldn't be better to either add better icon support to xwiki-rendering or to override these macros in xwiki-platform. I think we should first discuss how to continue with icons before merging this.

IMO overriding those is probably easier but also worse for long term technical debt.
I don't know how hard it would be to add better icon support in xwiki-rendering...
Leaving this PR as a draft until we finish the discussion and implement one of those two.

…uished by colors * Implementing the IconProvider interface to override xwiki-rendering behavior TODO: use a similar code to the icon nacro to provide the raw content of the block

…uished by colors * Implemented an alternative to isntantiate icons in rendering * Updated the POM * Fixed the components.txt file Note: I decided to not factorize witht the code in the DisplayIconMacro because we don't need anything related to a parameter.

Sereza7 · 2023-12-13T15:03:09Z

After these commits, here is what it looks like:
Silk vvv

Font Awesome vvv

TODO: update style to fit better with the prototype provided by tkrieck

manuelleduc · 2023-12-13T15:06:59Z

Nice! I haven't followed closely so the question might have been answered earlier but why isn't the icon aligned with the first line when the macro does not have a title?

Sereza7 · 2023-12-13T15:12:11Z

@manuelleduc
We discussed it with michitux a few days ago, it looked okay to me at first, but the conclusion was that I needed to review some of the style. I'll try to push a fix before the end of the day and share screenshots of the updated display (wanted to focus on the 'difficult' part which was creating a rendering API for icons).

…uished by colors * Started reflecting changes from rendering

…uished by colors * Updated position of the IconProvider to match the changes in rendering * Updated the return type of the iconProvider

Sereza7 · 2024-01-10T14:07:19Z

The canonical place for things related to xwiki-rendering-api would be xwiki-platform-rendering-xwiki.

Addressed in d906315 👍

I rebuilt all of the necessary modules successfully, and here is the result:

There is no UI change compared to before, except that we can see here icon transformation provided by two iconsets: font awesome when the mapping makes sense ( :) and (y)) and the default theme/Silk when it doesn't make much sense in other icon themes (:P and :D).

michitux

Thank you, overall this looks good just some updates to be done after the changes to the corresponding PR in xwiki-rendering.

michitux · 2024-01-16T14:21:02Z

...ering-xwiki/src/main/java/org/xwiki/rendering/internal/transformation/XWikiIconProvider.java

+    where we fall back on the default behaviour (the current icon theme does not have
+    the requested icon).
+     */
+    private static final List<Class> ICON_CLASS = new ArrayList<Class>(List.of(RawBlock.class, ImageBlock.class));


I don't think this is needed anymore.

Addressed in 341b4d9 👍

michitux · 2024-01-16T14:21:11Z

...ering-xwiki/src/main/java/org/xwiki/rendering/internal/transformation/XWikiIconProvider.java

+    /**
+     * @return the java class of the icon block created.
+     */
+    public List<Class> getIconClass()


I think this method should be removed.

Addressed in 341b4d9 👍

michitux · 2024-01-16T15:11:05Z

...ering-xwiki/src/main/java/org/xwiki/rendering/internal/transformation/XWikiIconProvider.java

+        try {
+            iconSet = getIconSet(iconName);
+            String iconContent = this.iconRenderer.renderHTML(iconName, iconSet);
+            return new IconBlock(List.of(new RawBlock(iconContent, Syntax.HTML_5_0)));


If you should adopt my suggestion to remove IconBlock from the API, this needs to be adjusted.

Addressed in 341b4d9 👍

michitux · 2024-01-16T15:12:04Z

...ering-xwiki/src/main/java/org/xwiki/rendering/internal/transformation/XWikiIconProvider.java

+ * 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.internal.transformation;


Why is this in a transformation package? This is not really related to transformations anymore I think.

The DefaultIconProvider from xwiki-rendering is in a transformation package. I want to inherit this class which is internal, so I figured I'd put it in the same package.
Moreover, there's no real place where this would suit perfectly, the best I could find from a quick look would be org.xwiki.rendering.internal.util.

For reference, the classes XWikiHTMLRawBlockFilter and XWikiErrorBlockGenerator are in a similar situation.

I'm not very confident in this choice, let me know where you'd put it instead.

Taking a look to see if it would make sense to fit the DefaultIconProvider in utils, and move this one afterwards. 👀

From what I can see, there's no dependency anymore to anything in the transformation package, so I'll be moving those classes to util and updating the components.txt files :)

Move done in e1dfba5 👍

* Updated the XWikiIconProvider class to fit the changes on `platform`.

…uished by colors * Updated the position of the XWikiIconProvider to reflect the move in `xwiki-rendering`. Now in the `util` package instead of being in the `transformation` package.

Sereza7 · 2024-01-17T10:13:13Z

Changes since #2590 (review):

Updated the content and position of the XWikiIconProvider class to fit the changes on xwiki-rendering. It's now in the util package instead of being in the transformation package.

Tests

After e1dfba5 and making sure all the changes in xwiki-rendering were built, successfully built:

mvn clean install -f xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-transformations/xwiki-platform-rendering-transformation-icon
mvn clean install -f xwiki-platform-core/xwiki-platform-rendering/xwiki-platform-rendering-xwiki

…uished by colors * Updated the `since` value

…uished by colors * in the inline version, reduced the white space between the border and the icon

Sereza7 · 2024-01-18T14:23:24Z

TODO: Add a sr-only alternative for the icon See https://forum.xwiki.org/t/updating-the-presentation-of-message-macros/13872/15?u=charpentierlucas

…uished by colors * Added a text alternative to the icons provided for boxes * Added default translations for the box icons. * Updated release numbers for master branch Note: This does not have any visual effect, but facilitates things for screen readers.

Sereza7 · 2024-01-22T16:53:10Z

Added a text alternative for icons in the messages in 655cc72. Those text alternatives are not needed for any visual users, so I decided to leave them as .sr-only alternatives. Here is what the structure and display looks like after these changes to the XWikiIconProvider :

Waiting for the CSS update to set back this PR status as ready for review.

…uished by colors * Changed margins and gaps between icon and borders * Removed borders from the message macro * Removed box shadow from the message macro * Added a visually distinguished left border to each type of message

tkrieck · 2024-01-29T14:10:54Z

I did the changes discussed in this topic with the proposed solution.

…uished by colors * Factorized some style * Added a space on the line `border: none`

Sereza7 · 2024-01-29T14:51:19Z

@tkrieck I brought small codestyle fixes in 4e5bee5 :

One of our CSS practices is that there's a space before all properties. You had some lines with border:none that did miss this space. Just realized that this was not yet in our codestyle... Adding it right now.
Factorized some of the styles in the above selectors that already existed. As a rule of thumb, if you copy/paste some code you should ask yourself if you can factorize it. This is important for long term: if we need to update this bit of code, instead of changing 4 lines with the same content, we'll just be able to change one line (and avoid creating inconsistencies by forgetting to change some among the 4 lines...) :)

tkrieck · 2024-01-29T15:54:28Z

@tkrieck I brought small codestyle fixes in 4e5bee5 :

One of our CSS practices is that there's a space before all properties. You had some lines with border:none that did miss this space. Just realized that this was not yet in our codestyle... Adding it right now.

Factorized some of the styles in the above selectors that already existed. As a rule of thumb, if you copy/paste some code you should ask yourself if you can factorize it. This is important for long term: if we need to update this bit of code, instead of changing 4 lines with the same content, we'll just be able to change one line (and avoid creating inconsistencies by forgetting to change some among the 4 lines...) :)

Got it, thank you!

… distinguished by colors (#2590)" This reverts commit 3bb55b5.

XWIKI-21452: Macros info, success, warning and error are only disting…

43cf09c

…uished by colors * Updated style to take into account the addition of an icon in xwiki-rendering

XWIKI-21452: Macros info, success, warning and error are only disting…

12d5de0

…uished by colors * Updated style to differentiate the title from the content of the box

Sereza7 marked this pull request as draft November 9, 2023 11:18

Sereza7 added 4 commits November 10, 2023 16:09

XWIKI-21452: Macros info, success, warning and error are only disting…

8959af0

…uished by colors * Updated style to align text in the box with a grid. Note: we don't know the exact height we want for this box (depends on content), so we can't use a column flexbox.

XWIKI-21452: Macros info, success, warning and error are only disting…

801d694

…uished by colors * Cleaned up style

XWIKI-21452: Macros info, success, warning and error are only disting…

bfb4467

…uished by colors * Updated padding and icon position for a cleaner presentation

XWIKI-21452: Macros info, success, warning and error are only disting…

8457d27

…uished by colors * Fixed width of the boxes fitted to content and icon alignement

Sereza7 marked this pull request as ready for review November 10, 2023 15:52

XWIKI-21452: Macros info, success, warning and error are only disting…

d6ed654

…uished by colors * Fixed presentation for inplace editing

Sereza7 mentioned this pull request Nov 17, 2023

XRENDERING-736: Macros info, success, warning and error are only distinguished by colors xwiki/xwiki-rendering#286

Merged

Sereza7 marked this pull request as draft December 8, 2023 16:52

Sereza7 and others added 3 commits December 12, 2023 17:23

XWIKI-21452: Macros info, success, warning and error are only disting…

1ea658a

…uished by colors * Implementing the IconProvider interface to override xwiki-rendering behavior TODO: use a similar code to the icon nacro to provide the raw content of the block

Merge branch 'xwiki:master' into XWIKI-21452

a382b4e

Sereza7 marked this pull request as ready for review January 5, 2024 15:10

Sereza7 marked this pull request as draft January 8, 2024 09:14

Sereza7 and others added 3 commits January 8, 2024 10:15

XWIKI-21452: Macros info, success, warning and error are only disting…

8818450

…uished by colors * Started reflecting changes from rendering

Merge branch 'xwiki:master' into XWIKI-21452

177566f

XWIKI-21452: Macros info, success, warning and error are only disting…

d906315

…uished by colors * Updated position of the IconProvider to match the changes in rendering * Updated the return type of the iconProvider

Sereza7 marked this pull request as ready for review January 10, 2024 14:07

michitux requested changes Jan 16, 2024

View reviewed changes

Sereza7 added 2 commits January 17, 2024 10:16

XWIKI-21009: LiveData alternative to drag controls

341b4d9

* Updated the XWikiIconProvider class to fit the changes on `platform`.

XWIKI-21452: Macros info, success, warning and error are only disting…

e1dfba5

…uished by colors * Updated the position of the XWikiIconProvider to reflect the move in `xwiki-rendering`. Now in the `util` package instead of being in the `transformation` package.

XWIKI-21452: Macros info, success, warning and error are only disting…

877c00f

…uished by colors * Updated the `since` value

michitux approved these changes Jan 17, 2024

View reviewed changes

XWIKI-21452: Macros info, success, warning and error are only disting…

e38416b

…uished by colors * in the inline version, reduced the white space between the border and the icon

Sereza7 marked this pull request as draft January 18, 2024 14:23

Sereza7 and others added 2 commits January 22, 2024 15:04

Merge branch 'xwiki:master' into XWIKI-21452

04b4ece

Sereza7 and others added 2 commits January 29, 2024 15:10

Merge branch 'xwiki:master' into XWIKI-21452

17c87ea

XWIKI-21452: Macros info, success, warning and error are only disting…

4e5bee5

…uished by colors * Factorized some style * Added a space on the line `border: none`

Sereza7 marked this pull request as ready for review January 29, 2024 14:39

michitux merged commit 3bb55b5 into xwiki:master Feb 15, 2024

michitux added a commit that referenced this pull request Feb 16, 2024

Revert "XWIKI-21452: Macros info, success, warning and error are only…

6174e46

… distinguished by colors (#2590)" This reverts commit 3bb55b5.

Sereza7 mentioned this pull request Mar 27, 2024

XWIKI-21452: Macros info, success, warning and error are only distinguished by colors #3023

Merged

Sereza7 mentioned this pull request Oct 25, 2024

XWIKI-22590: Links in message boxes lack contrast #3604

Merged

XWIKI-21452: Macros info, success, warning and error are only distinguished by colors #2590

XWIKI-21452: Macros info, success, warning and error are only distinguished by colors #2590

Conversation

Sereza7 commented Nov 8, 2023 • edited Loading

Jira

PR Changes

Tests

View

Note

vmassol commented Nov 9, 2023

tmortagne commented Nov 9, 2023 • edited Loading

Sereza7 commented Nov 9, 2023

tkrieck commented Nov 9, 2023

tmortagne commented Nov 9, 2023 • edited Loading

Sereza7 commented Nov 9, 2023

tkrieck commented Nov 9, 2023 • edited Loading

michitux commented Nov 9, 2023

tkrieck commented Nov 9, 2023

Sereza7 commented Nov 10, 2023

PR Changes

Note

View

Sereza7 commented Nov 15, 2023

michitux commented Dec 7, 2023

PR Changes

Note

View

Sereza7 commented Dec 8, 2023 • edited Loading

Sereza7 commented Dec 13, 2023 • edited Loading

manuelleduc commented Dec 13, 2023

Sereza7 commented Dec 13, 2023

Sereza7 commented Jan 10, 2024

michitux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sereza7 commented Jan 17, 2024

Tests

Sereza7 commented Jan 18, 2024

Sereza7 commented Jan 22, 2024 • edited Loading

tkrieck commented Jan 29, 2024

Sereza7 commented Jan 29, 2024 • edited Loading

tkrieck commented Jan 29, 2024

Sereza7 commented Nov 8, 2023 •

edited

Loading

tmortagne commented Nov 9, 2023 •

edited

Loading

tmortagne commented Nov 9, 2023 •

edited

Loading

tkrieck commented Nov 9, 2023 •

edited

Loading

Sereza7 commented Dec 8, 2023 •

edited

Loading

Sereza7 commented Dec 13, 2023 •

edited

Loading

Sereza7 commented Jan 22, 2024 •

edited

Loading

Sereza7 commented Jan 29, 2024 •

edited

Loading