[4.0] Template Atum - Change Alerts styling - Space saving #28974

coolcat-creations · 2020-05-06T19:06:37Z

Summary of Changes

Changed the Header of the message to be next to the message to save space
Decreased paddings
Changed colors

Testing Instructions

See if you like the new style and if not, tell me what you would prefer instead.

Before:

After:

thank you @dgrammatiko for the hints where to change everything :-)

dgrammatiko · 2020-05-06T19:13:21Z

maybe you can give me some hints were style changes for web components should be made

Definitely not inside the template's css!! 😉

This will help you visualise where the code should go

dgrammatiko · 2020-05-06T19:15:04Z

btw can you match the border radius of the input fields? This is way too aggressive

coolcat-creations · 2020-05-06T19:15:10Z

maybe you can give me some hints were style changes for web components should be made

Definitely not inside the template's css!! 😉

This will help you visualise where the code should go

Ah I was blind - also the joomla-alert.scss was not existing there :-) thanks a lot!!! Will change it in the next commit :-)

coolcat-creations · 2020-05-06T19:15:38Z

btw can you match the border radius of the input fields? This is way too aggressive

yes, will do, thanks!

dgrammatiko · 2020-05-06T19:17:05Z

Use this as your guide (it's the old code before moving to inline): https://github.com/joomla/joomla-cms/blob/08b27e74737f859c6a873c661df268ca4a7f148b/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss

coolcat-creations · 2020-05-06T19:27:37Z

Use this as your guide (it's the old code before moving to inline): https://github.com/joomla/joomla-cms/blob/08b27e74737f859c6a873c661df268ca4a7f148b/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss

how do I compile it @dgrammatiko ? npm run build:css and npm ci does not work...

dgrammatiko · 2020-05-06T19:31:19Z

You need to re add the file in the build tools:

I shouldn't have asked @ciar4n to remove it in the first place 😂

Add scss to the building process

coolcat-creations · 2020-05-06T19:39:07Z

@dgrammatiko thank you for your fast help on this, now the PR can be tested :-)

ciar4n · 2020-05-06T21:19:48Z

Should this styling extend to all alerts (e.g.. sample data plugin). If yes then would make more sense to have these changes upstream on the custom elements repo,

infograf768 · 2020-05-07T07:02:11Z

Text not vertically centered in admin login page.

If Persian is chosen for admin language at admin login time, the alert is in Persian but rtl is not respected (it is normal that texts are still in English as we have no Persian installation language yet.)

Page source

<!DOCTYPE html>
<html lang="fa-ir" dir="rtl">
<head>
	<meta charset="utf-8">
	<meta name="viewport" content="width=device-width, initial-scale=1">
	<meta name="theme-color" content="#1c3d5c">
	<meta name="generator" content="Joomla! - Open Source Content Management">
	<title>general - مدیریت</title>
	<link href="/testinstall/joomla40/administrator/templates/atum/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon">

	<link href="/testinstall/joomla40/administrator/templates/atum/css/vendor/fontawesome-free/fontawesome.min.css?5.13.0" rel="stylesheet" />
	<link href="/testinstall/joomla40/media/vendor/skipto/css/SkipTo.css?2.1.1" rel="stylesheet" />
	<link href="/testinstall/joomla40/administrator/templates/atum/css/template-rtl.min.css?1551912b2ae894eed233f36ef01d0b05" rel="stylesheet" />
	<link href="/testinstall/joomla40/administrator/language/fa-IR/fa-IR.css?1551912b2ae894eed233f36ef01d0b05" rel="stylesheet" />
	<link href="/testinstall/joomla40/administrator/templates/atum/css/vendor/joomla-custom-elements/joomla-alert.min.css?1551912b2ae894eed233f36ef01d0b05" rel="stylesheet" />

</head>

Also I find the text a bit small:
Good example:

1rem is more legible.

infograf768 · 2020-05-07T07:14:31Z

Should this styling extend to all alerts (e.g.. sample data plugin).

Looks these are fine here. Legible.

brianteeman · 2020-05-07T08:30:01Z

On a wide screen with a short message the close x is a long way from the text where the eye is focused - as I type this I realise that this is true in J3 as well - still this is an opportunity to change that

ciar4n · 2020-05-07T08:40:29Z

@infograf768 I'm just trying to make the point that we don't need multiple different styles of alerts. Whatever styling is chosen should be the same for all alerts unless there is good reason for otherwise.

coolcat-creations · 2020-05-07T09:06:03Z

Should this styling extend to all alerts (e.g.. sample data plugin). If yes then would make more sense to have these changes upstream on the custom elements repo,

Why are not all Alerts in the same styling? Thought the backend template would be the right place? Can you give me an advise what to do? I think styling should be done by the template...

coolcat-creations · 2020-05-07T09:08:16Z

@infograf768 thank you will test with RTL with the next changes too.

On a wide screen with a short message the close x is a long way from the text where the eye is focused - as I type this I realise that this is true in J3 as well - still this is an opportunity to change that

@brianteeman - How about a close text right after the message in a new line? Additionally not instead of the close icon at the top right?

brianteeman · 2020-05-07T09:20:30Z

I think you will need to check that with the accessibility team

dgrammatiko · 2020-05-07T09:22:54Z

I think styling should be done by the template...

It is done by the template, the file you just created affects only this template. The reasons why the css should not be inside the template.css are:

Cache busting, eg if the css is not changed there is no need to invalidate the cached file (updated BS doesn't mean that ALSO the custom elements are updated)
Joomla is modular by design, throwing everything in one file (monolithic approach) means we break the CMS' architecture

would make more sense to have these changes upstream on the custom elements repo

If you update the other repo styles it means that every template will get these styles, (eg cassiopeia and every other front end or back end template [given that they're not utilising an override]). I think leaving the default look as a close resemblance of the BS alerts will be the expected (eg most front end templates are, sadly, still based on BS)

How about a close text right after the message in a new line?

You don't need the text visible, the text exists for the screen readers but is not visible for the rest. Kinda expected behaviour

SharkyKZ · 2020-05-07T09:27:15Z

Should I open an issue if I get only these alerts instead of the ugly ones?

ciar4n · 2020-05-07T09:34:49Z

If you update the other repo styles it means that every template will get these styles

Fair point. In that case, I'd suggest not importing the CSS from the custom elements repo. Otherwise, you are largely styling the same element twice.

ciar4n · 2020-05-07T09:39:45Z

Why are not all Alerts in the same styling?

Your CSS is only been applied to the system alerts so all other alerts are using the custom-element repo CSS.

Thought the backend template would be the right place?

Sure. I'd suggest removing the import from the custom element repo (https://github.com/joomla/joomla-cms/pull/28974/files#diff-eb1f14c7c40e114d5c452c3f49b97355R2) and instead copy it to the template. Edit as needed instead of overriding.

coolcat-creations · 2020-05-07T09:47:48Z

I think styling should be done by the template...

It is done by the template, the file you just created affects only this template. The reasons why the css should not be inside the template.css are:
* Cache busting, eg if the css is not changed there is no need to invalidate the cached file (updated BS doesn't mean that ALSO the custom elements are updated)

* Joomla is modular by design, throwing everything in one file (monolithic approach) means we break the CMS' architecture

Sorry there was a misunderstanding, with "by the template" I meant inside atum, no matter if separate file or not. I think we are on the same track here :-)

would make more sense to have these changes upstream on the custom elements repo

If you update the other repo styles it means that every template will get these styles, (eg cassiopeia and every other front end or back end template [given that they're not utilising an override]). I think leaving the default look as a close resemblance of the BS alerts will be the expected (eg most front end templates are, sadly, still based on BS)

yes, agree on that, that's why I asked - I asked the question because I wondered why the styles I created where not applied to the other alerts. Since clarification from Ciaran I understand now :-)

How about a close text right after the message in a new line?

You don't need the text visible, the text exists for the screen readers but is not visible for the rest. Kinda expected behaviour

No I meant to add a close Link or Button right after the message as an alternate way to close the alert. Additionally to the cross at upper right. That's how some modals also are displayed these days...

coolcat-creations · 2020-05-07T09:50:20Z

Should I open an issue if I get only these alerts instead of the ugly ones?

I don't understand your issue :-) Did you run npm after applying the patch? Anyway - give me a bit time to change this PR towards something better. Not easy but I'll try :-)

SharkyKZ · 2020-05-07T11:23:49Z

I'm getting that before patch. I can't reproduce the "before" styling shown in this PR.

ciar4n · 2020-05-07T11:50:30Z

@coolcat-creations Your 'before' screenshot is incorrect. You will need to run node build.js --compile-css on your 4.0-dev branch to the get correct current alert styling.

nurcihandem · 2020-08-03T13:44:14Z

Changes are looking quite good so far, but some space might be needed between the X and the content. Centering the left headline vertically would be nice, too.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28974.}

nurcihandem · 2020-08-03T13:45:05Z

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28974.}

infograf768 · 2020-08-04T08:27:18Z

@coolcat-creations
Still needs 1rem instead of .8rem
i.e. in the resulting css

#system-message-container joomla-alert div {
    padding: .3rem 2rem;
    font-size: 1rem; // change
}

to center "Message" or "Warning", etc., I suggest:

#system-message-container joomla-alert .alert-heading {
    display: flex; // change from block
    padding: .8rem;
    font-size: 1rem; // added specially for login page
    color: rgba(255, 255, 255, 0.95);
    background: var(--alert-accent-color, transparent);
    justify-content: center; // added
    align-content: center; // added
    flex-direction: column; // added
}

Give some space around the close button

#system-message-container joomla-alert div p {
      margin: .5rem .5rem .5rem 0; } // modified
    html[dir=rtl] #system-message-container joomla-alert div p {
      margin: .5rem 0 .5rem .5rem; }  // added

infograf768 · 2020-08-04T10:46:46Z

BTW, I also found the reason why the message displayed in the login page is not similar (the alignment of the text value) to the ones in admin general.
What happens is that the system.message layout is not used. Instead is directly used /system/js/core.es6/message.es6
As that uses a span and does not include a <p>, the css is not applied correctly.

I'm pretty sure there is a simpler way than modifying the message.es6 to fit, i.e.
Change

      // Skip titles with untranslated strings
      if (typeof title !== 'undefined') {
        titleWrapper = document.createElement('span');
        titleWrapper.className = 'alert-heading';
        titleWrapper.innerHTML = Joomla.Text._(type) ? Joomla.Text._(type) : type;
        messagesBox.appendChild(titleWrapper);
      }
      
     //Add messages to the message box
      typeMessages.forEach((typeMessage) => {
        messageWrapper = document.createElement('div');
        messageWrapper.innerHTML = typeMessage;
        messagesBox.appendChild(messageWrapper);
      });

to

      // Skip titles with untranslated strings
      if (typeof title !== 'undefined') {
        titleWrapper = document.createElement('div');
        titleWrapper.className = 'alert-heading';
        titleWrapper.innerHTML = Joomla.Text._(type) ? Joomla.Text._(type) : type;
        messagesBox.appendChild(titleWrapper);
      }

      // Add messages to the message box
      typeMessages.forEach((typeMessage) => {
        messageWrapper = document.createElement('div');
        messageWrapper.innerHTML = '<p>' + typeMessage + '</p>';
        messagesBox.appendChild(messageWrapper);
      });

That would be for another PR but calling
@Fedik @dgrammatiko

Fedik · 2020-08-04T13:38:20Z

As that uses a span and does not include a <p>

hm, why we have a <p> there? It not safe to have it as message container. <p> does not allows other block element inside, and if message will contain extra html markup with <div> then whole markup will be broken. And I am sure some extensions will have it.

infograf768 · 2020-08-04T14:34:41Z

@Fedik
this the way the system.message layout is made

infograf768 · 2020-08-04T14:38:02Z

see https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/system/message.php#L51

and except for login.php, it is that layout which is used

Fedik · 2020-08-04T14:51:38Z

I think system.message layout need to be changed to something else than <p>

upd: it can be as in j3, a div with some class

joomla-cms/layouts/joomla/system/message.php

Lines 23 to 30 in 6e84eb7

    
           <?php if (!empty($msgs)) : ?> 
        
           	<h4 class="alert-heading"><?php echo JText::_($type); ?></h4> 
        
           	<div> 
        
           		<?php foreach ($msgs as $msg) : ?> 
        
           			<div class="alert-message"><?php echo $msg; ?></div> 
        
           		<?php endforeach; ?> 
        
           	</div> 
        
           <?php endif; ?>

infograf768 · 2020-08-04T15:08:53Z

Ok. this means modifying both the layout and the es6 file first before a refactoring of this PR here as css have to be changed.

Fedik · 2020-08-04T15:21:23Z

yeap, that should be easy I think

and your code for js also looks correct for <p>, for <div> it will be similar:

 // Add messages to the message box
typeMessages.forEach((typeMessage) => {
  messageWrapper = document.createElement('div');
  messageWrapper.innerHTML = `<div class="alert-message">${typeMessage}</div>`;
  messagesBox.appendChild(messageWrapper);
});

Fedik · 2020-08-04T15:23:17Z

I think it also can be part of this PR

infograf768 · 2020-08-04T15:23:30Z

yep. easy. will keep the div instead of the H4 in the layout

infograf768 · 2020-08-05T09:18:41Z

@coolcat-creations
Here is a full .diff containing your PR modified to use div instead of span or p (in layout and js) as well as the modifications in the scss for ltr and rtl.

j4alerts.diff.zip

diff --git a/administrator/templates/atum/scss/_variables.scss b/administrator/templates/atum/scss/_variables.scss
index 00d006d..0228e55 100644
--- a/administrator/templates/atum/scss/_variables.scss
+++ b/administrator/templates/atum/scss/_variables.scss
@@ -166,6 +166,6 @@
 // Alerts
 $state-success-text:               var(--alert-success);
-$state-success-bg:                 lighten($green-dark, 70%);
-$state-success-border:             lighten($green-dark, 30%);
+$state-success-bg:                 lighten($green-dark, 80%);
+$state-success-border:             $green-dark;
 
 $state-info-text:                  var(--white);
diff --git a/administrator/templates/atum/scss/blocks/_alerts.scss b/administrator/templates/atum/scss/blocks/_alerts.scss
index 0c2b33e..e1c9337 100644
--- a/administrator/templates/atum/scss/blocks/_alerts.scss
+++ b/administrator/templates/atum/scss/blocks/_alerts.scss
@@ -39,5 +39,5 @@
 
 .alert-heading {
-  font-size: $font-size-lg;
+  font-size: $h4-font-size;
 }
 
@@ -53,2 +53,3 @@
   }
 }
+
diff --git a/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss b/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss
new file mode 100644
index 0000000..4504df4
--- /dev/null
+++ b/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss
@@ -0,0 +1,84 @@
+@import "../../variables";
+@import "../../../../../../node_modules/joomla-ui-custom-elements/dist/css/joomla-alert";
+
+// The following is a restyle for the system alerts
+
+#system-message-container joomla-alert {
+  position: relative;
+  width:100%;
+  display: flex;
+  min-width: 16rem;
+  padding: 0;
+  margin-bottom: 1rem;
+  color: var(--atum-text-dark);
+  background-color: var(--white);
+  border: 1px solid var(--alert-accent-color, transparent);
+  border-radius: .25rem;
+  transition: opacity .15s linear;
+
+  .alert-heading {
+    display: flex;
+    padding: .8rem;
+    color: rgba(255, 255, 255, 0.95);
+    background: var(--alert-accent-color, transparent);
+    justify-content: center;
+    align-content: center;
+    flex-direction: column;
+  }
+
+  .alert-link {
+    color: var(--success, inherit);
+  }
+
+  &[type="success"] {
+    --alert-accent-color: var(--success);
+  }
+
+  &[type="info"] {
+    --alert-accent-color: var(--info);
+  }
+
+  &[type="warning"] {
+    --alert-accent-color: var(--warning);
+  }
+
+  &[type="danger"] {
+    --alert-accent-color: var(--danger);
+  }
+
+  .joomla-alert--close,
+  .joomla-alert-button--close {
+    position: absolute;
+    top: 0;
+    right: .3rem;
+    font-size: 2rem;
+    color: var(--alert-accent-color);
+    background: none;
+    border: 0;
+    opacity: 1;
+
+    &:hover,
+    &:focus {
+      text-decoration: none;
+      cursor: pointer;
+      opacity: .75;
+    }
+
+    [dir=rtl] & {
+      right: auto;
+      left: 0;
+    }
+  }
+
+  div {
+    font-size: 1rem;
+    .alert-message {
+      padding: .3rem 2rem .3rem .3rem;
+      margin: .5rem;
+
+      [dir=rtl] & {
+        padding: .3rem .3rem .3rem 2rem;
+      }
+    }
+  }
+}
diff --git a/build/build-modules-js/compilecss.es6.js b/build/build-modules-js/compilecss.es6.js
index 2c15e02..c076f7f 100644
--- a/build/build-modules-js/compilecss.es6.js
+++ b/build/build-modules-js/compilecss.es6.js
@@ -53,4 +53,5 @@
           `${RootPath}/administrator/templates/atum/scss/vendor/choicesjs/choices.scss`,
           `${RootPath}/administrator/templates/atum/scss/vendor/minicolors/minicolors.scss`,
+          `${RootPath}/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss`,
           `${RootPath}/administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-tab.scss`,
           `${RootPath}/administrator/templates/atum/scss/vendor/fontawesome-free/fontawesome.scss`,
diff --git a/build/media_source/system/js/core.es6/message.es6 b/build/media_source/system/js/core.es6/message.es6
index f0144da..981cd42 100644
--- a/build/media_source/system/js/core.es6/message.es6
+++ b/build/media_source/system/js/core.es6/message.es6
@@ -76,5 +76,5 @@
       // Skip titles with untranslated strings
       if (typeof title !== 'undefined') {
-        titleWrapper = document.createElement('span');
+        titleWrapper = document.createElement('div');
         titleWrapper.className = 'alert-heading';
         titleWrapper.innerHTML = Joomla.Text._(type) ? Joomla.Text._(type) : type;
@@ -85,5 +85,5 @@
       typeMessages.forEach((typeMessage) => {
         messageWrapper = document.createElement('div');
-        messageWrapper.innerHTML = typeMessage;
+        messageWrapper.innerHTML = `<div class="alert-message">${typeMessage}</div>`;
         messagesBox.appendChild(messageWrapper);
       });
diff --git a/layouts/joomla/system/message.php b/layouts/joomla/system/message.php
index d102f98..a319f23 100644
--- a/layouts/joomla/system/message.php
+++ b/layouts/joomla/system/message.php
@@ -49,5 +49,5 @@
 						<div>
 							<?php foreach ($msgs as $msg) : ?>
-								<p><?php echo $msg; ?></p>
+								<div class="alert-message"><?php echo $msg; ?></div>
 							<?php endforeach; ?>
 						</div>

coolcat-creations · 2020-08-05T13:51:39Z

@infograf768 thank you so much for your help on this. In the moment I am very occupied in teaching a class of kids and I have no clue how to merge your diff. Can you create a new PR and I will close this one? That would help me very much. Thanks a lot!!!

infograf768 · 2020-08-05T16:48:34Z

Thanks @coolcat-creations

here is the PR to test: #30294
closing this one now.

Change Alers styling

b4077a7

joomla-cms-bot added the PR-4.0-dev label May 6, 2020

Move css to the right place

403614a

Add scss to the building process

Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jun 3, 2020

infograf768 mentioned this pull request Aug 5, 2020

[4.0] Atum and Cassiopea Change alerts styling - space saving + #30294

Merged

infograf768 closed this Aug 5, 2020

joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Aug 5, 2020

[4.0] Template Atum - Change Alerts styling - Space saving #28974

[4.0] Template Atum - Change Alerts styling - Space saving #28974

Conversation

coolcat-creations commented May 6, 2020 • edited

Summary of Changes

Testing Instructions

dgrammatiko commented May 6, 2020

dgrammatiko commented May 6, 2020

coolcat-creations commented May 6, 2020

coolcat-creations commented May 6, 2020

dgrammatiko commented May 6, 2020

coolcat-creations commented May 6, 2020

dgrammatiko commented May 6, 2020

coolcat-creations commented May 6, 2020

ciar4n commented May 6, 2020 • edited

infograf768 commented May 7, 2020 • edited

infograf768 commented May 7, 2020

brianteeman commented May 7, 2020

ciar4n commented May 7, 2020

coolcat-creations commented May 7, 2020

coolcat-creations commented May 7, 2020

brianteeman commented May 7, 2020

dgrammatiko commented May 7, 2020

SharkyKZ commented May 7, 2020

ciar4n commented May 7, 2020

ciar4n commented May 7, 2020

coolcat-creations commented May 7, 2020

coolcat-creations commented May 7, 2020

SharkyKZ commented May 7, 2020

ciar4n commented May 7, 2020

nurcihandem commented Aug 3, 2020

nurcihandem commented Aug 3, 2020

infograf768 commented Aug 4, 2020 • edited

infograf768 commented Aug 4, 2020 • edited

Fedik commented Aug 4, 2020 • edited

infograf768 commented Aug 4, 2020

infograf768 commented Aug 4, 2020

Fedik commented Aug 4, 2020 • edited

infograf768 commented Aug 4, 2020

Fedik commented Aug 4, 2020

Fedik commented Aug 4, 2020

infograf768 commented Aug 4, 2020 • edited

infograf768 commented Aug 5, 2020

coolcat-creations commented Aug 5, 2020

infograf768 commented Aug 5, 2020

coolcat-creations commented May 6, 2020 •

edited

ciar4n commented May 6, 2020 •

edited

infograf768 commented May 7, 2020 •

edited

infograf768 commented Aug 4, 2020 •

edited

infograf768 commented Aug 4, 2020 •

edited

Fedik commented Aug 4, 2020 •

edited

Fedik commented Aug 4, 2020 •

edited

infograf768 commented Aug 4, 2020 •

edited