Bug 27695 - Fix style of messages on Elasticsearch configuration page
Summary: Fix style of messages on Elasticsearch configuration page
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-12 17:07 UTC by Owen Leonard
Modified: 2021-05-24 15:09 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 27695: Fix style of messages on Elasticsearch configuration page (4.72 KB, patch)
2021-02-12 19:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27695: Fix style of messages on Elasticsearch configuration page (4.77 KB, patch)
2021-02-24 21:15 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 27695: Fix style of messages on Elasticsearch configuration page (4.82 KB, patch)
2021-03-03 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27695: (follow-up) Add specificity to CSS changes (1.78 KB, patch)
2021-04-09 18:11 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27695: Fix style of messages on Elasticsearch configuration page (4.93 KB, patch)
2021-04-14 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27695: (follow-up) Add specificity to CSS changes (1.85 KB, patch)
2021-04-14 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2021-02-12 17:07:40 UTC
There some error messages on the Elasticsearch configuration page which are unstyled because they're given a class ("error") which doesn't exist in the CSS.

There is also a message which I think should be styled as a note since it contains instructions which are always present.
Comment 1 Owen Leonard 2021-02-12 19:05:33 UTC
Created attachment 116842 [details] [review]
Bug 27695: Fix style of messages on Elasticsearch configuration page

This patch makes minor modifications to SCSS and HTML to make messages
on the Elasticsearch configuration page more consistent with other
interfaces.

To test you should have Elasticsearch selected as your search engine.

- Apply the patch and rebuild the staff interface CSS
  (https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)
- Go to Administration -> Search engine configuration (Elasticsearch)
- You should see a full-width "note" style message under the page
  heading, "Warning: Any changes..."
- Click the "Reset mappings" button at the bottom of the page. The
  confirmation alert you see should appear above the note-style message.
- I'm not sure how to trigger an "error" type message on this page, but
  you can test the updated style by going to Administration ->
  Currencies and submit a new currency with the same name and symbol as
  one which already exists.
Comment 2 Michal Denar 2021-02-24 21:15:02 UTC
Created attachment 117282 [details] [review]
Bug 27695: Fix style of messages on Elasticsearch configuration page

This patch makes minor modifications to SCSS and HTML to make messages
on the Elasticsearch configuration page more consistent with other
interfaces.

To test you should have Elasticsearch selected as your search engine.

- Apply the patch and rebuild the staff interface CSS
  (https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)
- Go to Administration -> Search engine configuration (Elasticsearch)
- You should see a full-width "note" style message under the page
  heading, "Warning: Any changes..."
- Click the "Reset mappings" button at the bottom of the page. The
  confirmation alert you see should appear above the note-style message.
- I'm not sure how to trigger an "error" type message on this page, but
  you can test the updated style by going to Administration ->
  Currencies and submit a new currency with the same name and symbol as
  one which already exists.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 3 Martin Renvoize 2021-03-03 10:59:24 UTC
Hmm.. are 'alert' and an 'error' really the same...?

I feel like we should have a clearer global idea of what 'message', 'info', 'alert', 'error' etc mean.. and I'm not even sure that's the definitive list :|
Comment 4 Owen Leonard 2021-03-03 12:35:01 UTC
(In reply to Martin Renvoize from comment #3)
> Hmm.. are 'alert' and an 'error' really the same...?

I don't know that they are from the point of view of the user.

At some point many scripts where modified to use a different method for sending messages to the template. My searches are failing me so I can't find the relevant bug number.

When this change was made, many scripts were updated so that the template would display a dialog with an "error" class which was not defined at the time:

push @messages, {type => 'error', code => 'error_on_delete' };

From the way this change was made I suspect the use of "error" was intended to serve the same purpose as "alert." In the staff client there are only "message" and "alert" type dialogs in use.

In some places the script has been changed to use the previously-standard "alert," e.g. Bug 22575.
Comment 5 Owen Leonard 2021-03-03 12:45:57 UTC
For what it's worth...

The "message" style was originally defined for any kind of message that is informational, i.e. no action is required on the part of the user.

<div class="dialog message"> ... </div>

The "alert" style was intended for messages which DO require the user  to take action: They have to make a choice asked in the dialog, or they have to address the error in the dialog in order to proceed.

<div class="dialog alert"> ... </div>

The "error" parameter being added to scripts resulted in this markup:

<div class="dialog error"> ... </div>

...which was never added as a style in CSS.
Comment 6 Martin Renvoize 2021-03-03 12:46:39 UTC
Thanks for clarifying Owen.. so this patch will actually fix a whole series of 'error' type dialogue boxes.. cool.
Comment 7 Martin Renvoize 2021-03-03 12:50:21 UTC
I was imagining we could go with something along the lines of Bootstraps native set of colour options..

primary, success, info, warning, danger

But these don't entirely lend themselves to our use case here.
Comment 8 Martin Renvoize 2021-03-03 12:52:06 UTC
Created attachment 117569 [details] [review]
Bug 27695: Fix style of messages on Elasticsearch configuration page

This patch makes minor modifications to SCSS and HTML to make messages
on the Elasticsearch configuration page more consistent with other
interfaces.

To test you should have Elasticsearch selected as your search engine.

- Apply the patch and rebuild the staff interface CSS
  (https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)
- Go to Administration -> Search engine configuration (Elasticsearch)
- You should see a full-width "note" style message under the page
  heading, "Warning: Any changes..."
- Click the "Reset mappings" button at the bottom of the page. The
  confirmation alert you see should appear above the note-style message.
- I'm not sure how to trigger an "error" type message on this page, but
  you can test the updated style by going to Administration ->
  Currencies and submit a new currency with the same name and symbol as
  one which already exists.

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-03-03 12:53:48 UTC
After our discussion, I'm happy to call this PQA.. It fixes a class of missing styling in one hit.  I still think we could perhaps make a distinction between 'needs attention from staff user' and 'needs attention from administrator' (error vs alert) later down the line perhaps.

Passing QA
Comment 10 Owen Leonard 2021-03-03 13:13:10 UTC
(In reply to Martin Renvoize from comment #9)
> I still think we could perhaps make a
> distinction between 'needs attention from staff user' and 'needs attention
> from administrator' (error vs alert) later down the line perhaps.

I agree. I was going to suggest that the distinction be made between "requires the user to make a choice in order to proceed," and "the operation cannot be completed at all for some reason."
Comment 11 Martin Renvoize 2021-03-04 07:24:28 UTC
(In reply to Owen Leonard from comment #10) 
> I agree. I was going to suggest that the distinction be made between
> "requires the user to make a choice in order to proceed," and "the operation
> cannot be completed at all for some reason."

I've added it to the dev meeting agenda, thanks Owen :)
Comment 12 Jonathan Druart 2021-03-04 09:10:37 UTC
(In reply to Martin Renvoize from comment #6)
> Thanks for clarifying Owen.. so this patch will actually fix a whole series
> of 'error' type dialogue boxes.. cool.

Well, yes, maybe. or not.

$ git grep 'class="error"'
returns a long non-exhaustive list

Quick random test:
https://snipboard.io/px5fq4.jpg

When receiving,
before: https://snipboard.io/jSWMmf.jpg
after: https://snipboard.io/AOoUpa.jpg

And certainly many more. I don't think we should do that on this bug report.
Comment 13 Martin Renvoize 2021-03-04 14:10:38 UTC
Oops, my bad, I misread the patch.

I think this should only apply to 'message boxes' for the moment..


i.e.

.message .alert, .message .error 

?
Comment 14 Owen Leonard 2021-04-09 18:11:37 UTC
Created attachment 119428 [details] [review]
Bug 27695: (follow-up) Add specificity to CSS changes

This patch moves the definition of .alert and .error classes into the
block for .dialog in order to make sure they don't have an effect beyond
the desired scope.

The ".closebtn" section is redundant and has been removed.
Comment 15 Jonathan Druart 2021-04-14 08:47:59 UTC
Created attachment 119550 [details] [review]
Bug 27695: Fix style of messages on Elasticsearch configuration page

This patch makes minor modifications to SCSS and HTML to make messages
on the Elasticsearch configuration page more consistent with other
interfaces.

To test you should have Elasticsearch selected as your search engine.

- Apply the patch and rebuild the staff interface CSS
  (https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)
- Go to Administration -> Search engine configuration (Elasticsearch)
- You should see a full-width "note" style message under the page
  heading, "Warning: Any changes..."
- Click the "Reset mappings" button at the bottom of the page. The
  confirmation alert you see should appear above the note-style message.
- I'm not sure how to trigger an "error" type message on this page, but
  you can test the updated style by going to Administration ->
  Currencies and submit a new currency with the same name and symbol as
  one which already exists.

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2021-04-14 08:48:03 UTC
Created attachment 119551 [details] [review]
Bug 27695: (follow-up) Add specificity to CSS changes

This patch moves the definition of .alert and .error classes into the
block for .dialog in order to make sure they don't have an effect beyond
the desired scope.

The ".closebtn" section is redundant and has been removed.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2021-04-14 13:36:10 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 Fridolin Somers 2021-04-29 09:14:22 UTC
Pushed to 20.11.x for 20.11.06
Comment 19 Andrew Fuerste-Henry 2021-05-24 14:59:25 UTC
Pushed to 20.05.x for 20.05.12
Comment 20 Victor Grousset/tuxayo 2021-05-24 15:09:33 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.