Remove the slogan in install.php and update.php

Fixed markup for site slogan, any suggestions are welcome. Attached screenshots for reference.

Looks good!

Thanks for the patch. I need a little bit more time to review it. Having trouble with my local install

Wait, why do we even need a slogan here?

@Bojhan, true, but if it's printing right now then we might as well fix it.

The alignment of the slogan and the site name look slightly off.

The spacing between the two was irking me for some reason, in my eyes it looked a lot better with the following:

.name-and-slogan .site-slogan {
  margin: 0 1em 0 4.1em;
}

Did the changes as suggested in #6. Attaching new patch and interdiff.

@Lewis Well I dont really understand fixing something that shouldn't be there in the first place.

#7 didn't work because set margins were overridden later.

Fixing this seems busy-work. Slogan has no place here. Do we re-focus this issue on removing it or close this and open a new one? I'd prefer just removing it in this one.

Alright I'm allowed to change my mind :P Let's throw it out.

StevenPatz's patch effectively removes the slogan. The template has a condition checking for the site slogan presence. The condition first checks for site name which pretty much will always be set. If it can be accounted as code clean up, this patch removes the check for site slogan.

+++ b/core/themes/seven/templates/maintenance-page.html.twig
@@ -12,14 +12,11 @@
+    {% if site_name %}
       <div class="name-and-slogan">
         {% if site_name %}

I don't think we need that second if any more because it doesn't catch anything.

Remember to change the status to Needs review after sending a patch, otherwise the testbot doesn't trigger.

Looks like this needs a reroll?

+++ b/core/themes/seven/templates/maintenance-page.html.twig
@@ -12,14 +12,9 @@
       <div class="name-and-slogan">

I guess we can remove this div and class as well?

+++ b/core/themes/seven/templates/maintenance-page.html.twig
@@ -12,15 +12,8 @@
           <h1>{{ site_name }}</h1>

This line is intended wrong. After fixing this we're done :)

Is it one or two spaces? I'll have a reroll in a few seconds ( or later, not sure if the coder loung at DrupalCon is closing )

@StevenPatz It's two spaces.

I am a new contributor at the contribution mentoring at Drupal Con. I am going to work on this issue to get more acquainted with the contribution process.

From comment #20, we do need the wrapper div with the class because the selector used to style the title counts on that. It is .name-and-slogan h1 This new patch adds it back and removed the second condition mentioned in comment #16.

Edit: lahoosascoots, I had created the patch before seeing your comment. Please review mine and let's get this one in core :)

No worries, dinarcon. Reviewed the patch from #26 and it looks good. Going to leave the issue status as is until a more experienced contributor double checks my reviews.

Applies cleanly. Removing "needs re-roll tag".

-t

@dinarcon Can we add the class to the h1 instead and remove the wrapper?

Working on it.

@LewisNyman, sure. That requires updates to the styles though. I went ahead and did it by replacing the .name-and-slogan h1 selector by h1.page-title where appropriate. Please review.

@jstoller, manual review would be helpful.

I am testing this now.

Patch applied cleanly and verified. See attached screenshot.

Tested, applied fine, slogan markup is gone.

Image attached.

RTBC +1

Looks like slogan is still missing...

@daviddemello, there was no design for the slogan and it needs to be removed. We are removing it in this patch.

Also confirmed, see before & after images

@dinarcon: Margin was wrong for small screen sizes. Also, using h1 in the CSS was unnecessary specificity. The attached patch fixes both of these issues.

Responsive screen shots

#40 applied fine, specificity of the H1 was removed.

RTBC +1

Good catch @jstoller. Patch applies cleanly.

On responsive screenshots by @daviddemello I saw that long titles are cropped by the viewport. I added a css rule for it to wrap.

Note: In the interdiff you can see there is are margin-right and margin-bottom rules. I didn't use a shorthand because that is inheriting a margin top and bottom from another rule. It can be merged if considered properly.

Applied patch and did not see slogan when viewing screen using different sizes.

For some reason you patch will remove the CSS from the "Drupal" header from the installer. `

Thanks @darol100. The installer uses the same stylesheet and that's why it breaks. During installation the slogan won't be set so we could update that template too to remove the slogan.

Working on a new patch.

This is the new patch. Please review it @darol100

@dinarcon

Everything seem to be working fine now.

The installation header is working fine....

And the patch seem to be working....

Great Job

This just has two minor changes. On small screens, I increased the right margin on .page-title from 1.5em to 2em, so the text never bumps up against the step indicator. And on large screens, I added a right margin to .page-title, so long titles don't run up against the side of the install box. This made the special handling for RTL unnecessary

Applied patch. Reinstalled Drupal, code on install page relating to header/slogan matches that on the update page.
Long page titles wrap and display nicely on view ports from 300px wide to 1920px.

Thanks @jstoller. Updated styles for long names to break on word for desktop too.

Patch from #53 applies OK. Crazy long words in titles wrap much nicer.
Before and after small screen caps attached.

RTBC +1

Applied latest patch, before and after confirm slogan removed and styles for site name (header) correct

+++ b/core/themes/seven/maintenance-page.css
@@ -118,8 +119,9 @@
+  .page-title {
+    margin-right: 2em;
+    margin-bottom: 0.725em;
   }

@@ -174,11 +176,7 @@
+  .page-title {
...
   }

Has rtl been considered and tested?

Also bartik and system have maintenance and install css and twig files. What should we be doing here?

Working on the reroll.

Here it goes. Patch is basically the same. Interdiff was empty.

Let's activate our lovely test bot!

RTL is not yet working for the title and the step indicator. The step list's right padding is also off. But that's an another issue. I'll fix these and create a new issue.

Agreed, it needs it's own issue. The original issue has been fixed a couple patches back.

The #59 patch applies without issue and the output looks good with Simplytest.me

Here's the patch and the working mobile view. Please review!

Alex also pointed out that we need to remove the slogan from the following files as well:

/core/modules/system/templates/maintenance-page.html.twig
/core/themes/bartik/templates/maintenance-page.html.twig
/core/modules/system/templates/install-page.html.twig

Sure... I'll add them in. I also added the mentioned related issue.

Why is there a System install-page.html.twig and a Seven install-page.html.twig? Is there a scenario where Seven wouldn't be used? can an install profile disable Seven during install? or is it just for continuity/fallback?

@HaloFX Yes, in distributions. Also Seven requires special mark up in the design and as a standard we don't enforce the same mark up on every theme

This is getting quite complex. When I stripped out some markup I needed to refactor the CSS also. There's already a lot of going on.

Some pointers:

1. Changed .page-title to .site-name.
2. Changed some IDs to classses to reduce specifity.
3. Tried to make the site-name more SMACSS. Needs more work but better to do that in another issue.

I tried to test all the things I could think of.

This issue is related to the default install and update pages in a drupal 8 core installation, not in the context of a distribution. As though, we could assume that Seven will be used as the admin theme. @alexpott, in that case I think there should not be any change required to bartik's and system's files as asked in https://drupal.org/node/2230997#comment-8870607

IMHO and as mentioned a couple of times, the original issue has already been fixed. @jiv_e's patch at https://drupal.org/node/2230997#comment-8880875 works. I include a new patch that updates a CSS rule for consistency with other rules in the file.

I have updated the issue summary. Please let me know if I missed something.

Tested patch from #71

Applies clean and works with both LTR and RTL

1. +++ b/core/themes/seven/maintenance-page.css
   @@ -39,7 +40,7 @@
        right: auto;
   

   Can be we add the missing /* LTR */ comment to the correspond CSS above.

2. +++ b/core/themes/seven/maintenance-page.css
   @@ -118,9 +119,16 @@
   +    margin-right: 2em;
   +    margin-bottom: 0.725em;
   

   Missing /* LTR */ comment

Thanks Alex, I found a few more lines in that file that were missing LTR comments.

I also changed the step indicator RTL value match the LTR value.

Applied latest patch (#73) against current HEAD.

Still applies cleanly, appropriate LTR comments present, step margin difference so small it's hard to capture in a screenshot, as such no updated screenshots included, previous set still basically the same.

Committed 8b9d600 and pushed to 8.x. Thanks!

diff --git a/core/themes/seven/maintenance-page.css b/core/themes/seven/maintenance-page.css
index 61394fa..213c134 100644
--- a/core/themes/seven/maintenance-page.css
+++ b/core/themes/seven/maintenance-page.css
@@ -123,12 +123,10 @@
     margin-right: 2em; /* LTR */
     margin-bottom: 0.725em;
   }
-
   [dir="rtl"] .page-title {
     margin-right: 0;
     margin-left: 2em;
   }
-
 }
 
 @media all and (min-width: 48em) { /* 768px */

Removed some blank lines added by patch on commit.