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.

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

[UX] Support a 4th "info" type for backdrop_set_message #3155

Closed
klonos opened this issue May 30, 2018 · 44 comments
Closed

[UX] Support a 4th "info" type for backdrop_set_message #3155

klonos opened this issue May 30, 2018 · 44 comments

Comments

@klonos
Copy link
Member

klonos commented May 30, 2018

https://api.backdropcms.org/api/backdrop/core%21includes%21bootstrap.inc/function/backdrop_set_message/1

...we currently support only the following message types:

  • status:

    screen shot 2018-05-31 at 5 44 58 am

  • warning:

    screen shot 2018-05-31 at 5 45 43 am

  • error:

    screen shot 2018-05-31 at 5 44 19 am

I propose supporting another type of info:

screen shot 2018-05-31 at 5 50 03 am

I think that by doing this, we would also allow contrib modules to communicate info things in a standardised manner. Think Drupal webform module and the message that was added recently:

screen shot 2018-05-31 at 5 53 28 am

(This should not affect our backwards compatibility, as it seems like an API addition.)


PR by @klonos: backdrop/backdrop#2217
PR by @jenlampton: backdrop/backdrop#2291

@opi
Copy link

opi commented May 31, 2018

Agreed. I suggest we use an "Info" picto, like https://fontawesome.com/icons/info-circle?style=solid

@opi
Copy link

opi commented May 31, 2018

Note that in backdrop_set_message and backdrop_get_message, the $type argument is never checked, nor in theme_status_messages where the type is printed out as CSS class, without check or sanitization.
It means the implementation is only on the admin theme side, and documentation.

@klonos
Copy link
Member Author

klonos commented May 31, 2018

I suggest we use an "Info" picto, like https://fontawesome.com/icons/info-circle?style=solid

Of course we should! ...I was lazy when I was putting the mockups together :)

@klonos
Copy link
Member Author

klonos commented Jun 1, 2018

PR just filed: backdrop/backdrop#2217

Unfortunately, the info type for messages is not being used anywhere in core at the time being, so you cannot test this in the sandbox. For those that cannot test locally, here's how it looks:

screen shot 2018-06-02 at 7 20 53 am

@docwilmot
Copy link
Contributor

Is there anywhere in core now where this could or should be used? If the answer is "no" I suspect we should be questioning if this is necessary.

@klonos
Copy link
Member Author

klonos commented Jun 3, 2018

@docwilmot yes, I think I have mentioned this in another issue recently. Here are a few examples where messages we throw at the user are either not using backdrop_set message when they should've:

screen shot 2018-06-04 at 6 20 46 am

...or are using the status/success message type, when the user has not performed any directly related action:

screen shot 2018-06-04 at 6 23 15 am

^^ this is visiting the module installer page, after having installed modules that were intentionally left disabled. I have just intentionally navigated back to the Project Installer page with the intention to install additional modules that I forgot to add in the first go.

...this specific message persists across other project installer sections as status/success. Here's the same message, talking about recently installed modules, while I'm at the layouts installation page:

screen shot 2018-06-04 at 6 33 17 am

^^ this can a) either be considered a bug (in which case we should make sure that this message only shows in the modules installer page), or b) considered a feature (in which case the message could use the new 'info' type).

Here's another example:

screen shot 2018-06-04 at 6 40 24 am

^^ ...layout templates are automatically enabled during the last step of the installer (unlike modules and themes). The first part of this message is correctly thrown as a success, the last part should be an 'info'. Separating this message to a success and an info one, would allow us to dismiss the one, while keeping the second one persistent...

...here's how this story continues, if you navigate away from the layout templates page and install a theme:

screen shot 2018-06-03 at 11 20 41 am

^^ this is where my previous suggestion makes sense. The first message now seems like a bug, unless we have only kept the "You can manage layout templates on the layouts Settings page." as a persistent 'info' message. Also, the part that is placed at the top of the list of themes as plain text feels like it could also be a separate 'info' message.

There's other examples where this could be used in core, but contrib can also use it to convey messages like additional manual steps that might be required after upgrading to a major version etc.. In general, messages that need to be shown as help text, but unlike actual help text, should not be shown all the time.

@docwilmot
Copy link
Contributor

I'm not clear on the difference between status and info though. Can we have it explicitly described and agreed on in preparation for this?

@klonos
Copy link
Member Author

klonos commented Jun 3, 2018

Sure. Here's my take on this...

"Sophisticated" definitions:

  • Status/Success message: something that needs to be displayed to the user as a direct result of the action they were performing. Shown once, on the page where the action finishes.
  • Help/Info message: something that needs to be displayed to the user persistently across various pages. Can be the result of their actions on another section of the site (excludes messages that should be warnings/errors).

(overly) simplified rules:

  • Status/Success message: Yay! It worked.
  • Help/Info message: Did you know ...??

Would be interested to know what others think.

@klonos
Copy link
Member Author

klonos commented Jun 3, 2018

...having said that, I personally think that although it is good to define/document these, they should not be strict/explicit. The success messages have been abused in many cases over the years, both in Drupal and in Backdrop. The introduction of the new 'info' message type should be a way to better the situation and clarify things IMO. A means to expend and provide more options; not a means to impose and restrict.

@opi
Copy link

opi commented Jun 4, 2018

I'm not clear on the difference between status and info though.

Probably because 'status' is more a 'success' message, with a green color theme and a positive check pictogram. Is it worth renaming ?

@opi
Copy link

opi commented Jun 4, 2018

Here is a full list of message display through backdrop_set_message() : https://gist.github.com/opi/f28a1ed0dc2e26b44b543b02026addb4 ; Not sure we will find so much to display as 'info'

@laryn
Copy link
Contributor

laryn commented Jun 4, 2018

It makes sense to me to have an "info" message separately from the "success/status" message, and @klonos's description of the difference is helpful.

Other examples of "persistent" info messages that should not have a checkmark/success indicator:

  • Your account is pending approval
  • There are no updates currently

@quicksketch quicksketch changed the title Support a 4th "info" type for backdrop_set_message [UX] Support a 4th "info" type for backdrop_set_message Jun 21, 2018
@jenlampton jenlampton added this to the 1.11.0 milestone Jun 21, 2018
@jenlampton
Copy link
Member

Yes please! Love this idea, thanks @klonos :)

@klonos
Copy link
Member Author

klonos commented Jun 25, 2018

@laryn ...this is my OCD talking again plus my personal preference for Backdrop CMS the product to be "intelligent", but I believe that the examples you have listed are not 100% fit for the use case. If a message has been triggered by a user action, then this should be a success message. The info type should be used in the case that we need to convey something additional to the user, or if the message we are trying to display has not been a direct consequence of the user's current action.

To be more precise...

  • in the case of the "Your account is pending approval" message, it should be a success message if it is shown to the user directly after they have created an account. If they revisit the page at a later point in time (not sure if that is possible in this specific case), then this message should change to an info type. So first time we show the message: success (convey "it worked!"), successive times we show the message: info (convey "still pending").

  • in the case of "There are no updates currently", it should be a success message if the user has just clicked the "check for updates" button, but if we need to show this message to the user again at a later point (they navigated away and have returned to the same page after a while for example), then this should be an info message this second time. In actuality, we show this to the users:

    screen shot 2018-06-26 at 8 01 21 am

    ...I see how this could be a "Checked available update data for 3 projects." success message (as opposed to a "something went wrong" error), followed by:

    • a possible "all projects are up to date" success message
    • a possible "updates found for x projects" info message
    • a possible "recommended updates found for x projects" warning
    • a possible "pending security updates found for x projects" error

@klonos
Copy link
Member Author

klonos commented Jun 26, 2018

...another example where this can be used in contrib:

screen shot 2018-06-26 at 10 25 16 am

The "Installation is complete." success message would suffice in this case, and the other 2 messages could be shortened to just "[module_name] does not provide any new functionality on its own but may be used by other modules.", and converted to the new info type 😉

@opi
Copy link

opi commented Jun 26, 2018

Filled a related issue: [DX] Use proper 'success' message type and deprecate the ambiguous 'status' type. #3179

@klonos
Copy link
Member Author

klonos commented Jul 1, 2018

Joomla has this "messages" section that lists messages that need to be communicated to the user:

screen shot 2018-07-02 at 6 46 07 am

...although some of these seem that they should be warnings, respective messages about stats collection, or unsupported php versions in upcoming Backdrop releases could use this new info message type (unsupported php version in the current installation should still be a warning/error).

@klonos
Copy link
Member Author

klonos commented Jul 1, 2018

...bootstrap also seems to have this 4th info alert type too:

screen shot 2018-07-02 at 6 58 20 am

@klonos
Copy link
Member Author

klonos commented Jul 1, 2018

Note to self (for when this gets merged): another place where this could be used: #2583 / #3050

@opi
Copy link

opi commented Aug 4, 2018

Add a request regarding accessibility of focused links. Easy to fix though

@klonos
Copy link
Member Author

klonos commented Aug 5, 2018

Thanks @opi 👍...for posterity and so that we get some feedback from others, I am posting your comment here too:

just realized that links has no hover or focus effect. This is not nice regarding accessibility.
The easiest could be to remove the text-decoration:

.messages a:hover,
.messages a:focus {
  text-decoration: none;
}

...although from what I see we are doing the opposite everywhere else in the admin UI: links are not underlined by default, and they get underlined on hover/focus. Hmm... let me give it a bit of thought and I will update the PR ...I will probably go with your suggestion @opi unless anyone else has better ideas. @wesruv?

@opi
Copy link

opi commented Aug 5, 2018

Actually, the "focus" rule is not needed here, because it's already defined globally (blue outline)

@quicksketch
Copy link
Member

from what I see we are doing the opposite everywhere else in the admin UI: links are not underlined by default, and they get underlined on hover/focus.

Although it's opposite, I think it's better than no hover effect. This looks so close!

@jenlampton
Copy link
Member

I've filed a new PR that includes the hover state: backdrop/backdrop#2291

@quicksketch
Copy link
Member

Looks great. I added one more commit to add @since 1.11.0 Added "info" as an acceptable $type parameter. to the docblock of backdrop_set_message().

I've merged backdrop/backdrop#2291 into 1.x for 1.11.0. Thanks @klonos, @opi, @docwilmot, and @jenlampton!

@klonos
Copy link
Member Author

klonos commented Sep 3, 2018

Thanks everybody for finishing this off 👍...I have plans for putting it into good use 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants