install-page.html.twig markup and CSS

Nice clean up!

+++ b/core/themes/seven/install-page.cssundefined
@@ -38,152 +66,141 @@ body.install-page {
-    max-width: 770px;
+   max-width: 96em;

Looks like these max-widths don't match up

+++ b/core/themes/seven/install-page.cssundefined
@@ -38,152 +66,141 @@ body.install-page {
+  .install-page [role="banner"] h1 {
+    margin: 0.75em 0 0.75em 2em; /* LTR */

The left margin is wrong, it should match 50px

+++ b/core/themes/seven/install-page.cssundefined
@@ -23,11 +21,41 @@
+.install-page [role="banner"] h1 {

You could argue that we don't even need the role="banner"

+++ b/core/themes/seven/install-page.cssundefined
@@ -23,11 +21,41 @@
+.install-page .l-container {
+  background: #fff;
+  width: auto;
+  border-radius: 5px;
+  box-shadow: 0 6px 12px rgba(0, 0, 0, 0.15);

The border radius shouldn't appear on narrow views, we also need some padding and margins to match the previous design.

The install process looks good, I noticed a few things on the “already installed” page.

I noticed this before I uninstalled, I guess we've lost those sidebar classes?

+++ b/core/themes/seven/install-page.cssundefined
@@ -41,152 +73,140 @@ body.install-page {
+  .install-page [role="main"] {
     -webkit-box-sizing: border-box;
     -moz-box-sizing: border-box;
     box-sizing: border-box;
     clear: none;
-    float: left;
-    padding-left: 50px; /* LTR */

Floating right makes the page look weird when there is no left sidebar. I guess we don't have the .no-sidebar class anymore?

+++ b/core/themes/seven/install-page.cssundefined
@@ -41,152 +73,140 @@ body.install-page {
-  ul{
-    padding: 15px;
-    margin: 0.25em 0 0.25em 0;

We need this ul declaration, I'm happy for it to stay this broad, it makes sense.

Fixed #11 issues.

There's a conflict with the "already installed page" and the normal installed page. We've moved the sidebar mark up below the main content mark up, which means we need to float the main content right in or for the sidebar to sit left of it.

I don't really want to pull in the .no-sidebar classes, the easiest way to fix this is to move the sidebar mark up back above the content. This is fine with me because the sidebar content is navigation.

Fixing...

This patch fixed #14 issue and seems to work also in "already installed" page.

This looks good! I'm happy with the code.

For me the latest patch doesn't apply... I couldn't simplytest it, neither.

Re-rolled.

#19: 2041793-19.patch queued for re-testing.

Testet patch and added missing curly brackets.

I can confirm that patch #22 passed manual testing

Update, noticed that MAIN i lacking some padding-right.

setting status as needs work.

See #2094445: Fix installer right padding - make it look balanced

Updated patch to include padding-right on main.

Please test and confirm it look good.

Patch #25 passed manual testing.

Added missing comment.

Removed the /* LTR */ comments they are not needed here. The padding is the same for a RTL website.

Sorry, I found some RTL issues.

1. +++ b/core/themes/seven/install-page.css
   @@ -41,147 +73,144 @@ body.install-page {
   +  [dir="rtl"] .install-page .l-container {
   +    padding: 1em 0 3em 2em;
   +  }
   

   There is no padding set on ltr, not sure why we are setting it on rtl.

2. +++ b/core/themes/seven/install-page.css
   @@ -41,147 +73,144 @@ body.install-page {
   +  [dir="rtl"] .install-page [role="main"] {
        float: right;
   +    padding-left: 0;
   +    padding-right: 2em;
      }
   

   We still need to remove the padding here for rtl

i am working on this

I fixed the paddings. What do you think about it?

Great, thanks!

#32: 2041793-32.patch queued for re-testing.

Here's my review, unfortunately this one is not ready yet. Important stuff up front followed by nitpicks.

1. +++ b/core/themes/seven/install-page.html.twig
   @@ -0,0 +1,73 @@
   + * @file
   + * Default theme implementation to display a Drupal installation page.
   ...
   +          {% if site_name %}
   +              <h1>{{ site_name }}</h1>
   +          {% endif %}
   ...
   +     {{ messages }}
   +
   +      {{ content }}
   ...
   +  </div>{# /.l-container #}
   +
   +
   +</body>
   

   The @file should probably say something like 'Seven's implementation to display…', and also the spacing (blank lines) and alignment (some errors) of this template is very different from the core install-page.html.twig. I think when diffing the two files it should be much more obvious what we are changing between the two templates, which after tweaking looks like just the markup order of the first sidebar and adding a class to the html tag.

2. +++ b/core/modules/system/templates/install-page.html.twig
   @@ -12,64 +13,57 @@
    <body class="install-page">
   ...
   +<div class="l-container">
   

   Should the .l-container div be indented one level within <body>? In the most recent patch, the seven install-page.html.twig does this but the system one doesn't. I would definitely lean towards indenting it but maybe there's a standard or convention I'm missing.

3. +++ b/core/themes/seven/install-page.css
   @@ -2,8 +2,6 @@
     * @file
     * Installation styling.
     *
   - * Unfortunately we have to make our styling quite strong, to override the
   - * .in-maintenance styling.
     */
   

   If we're removing this paragraph, we need to remove the blank '*' line above it as well.

4. +++ b/core/themes/seven/install-page.css
   @@ -23,17 +21,51 @@
   +/**
   + * Layout
   + *
   + */
   

   Extra blank '*' line here should be removed.

5. +++ b/core/themes/seven/install-page.css
   @@ -23,17 +21,51 @@
   +@media all and  (max-width: 48em) { /* 768px */
   

   Extra space between "and" and "(max-width…" can be removed.

6. +++ b/core/themes/seven/install-page.css
   @@ -23,17 +21,51 @@
   +  .install-page .l-container{
   

   Need a space before the { per CSS coding standards: https://drupal.org/node/1887862

7. +++ b/core/themes/seven/install-page.css
   @@ -41,147 +73,137 @@ body.install-page {
   +  /* Positioning  sidebar & content */
   

   Extra space here between "Positioning" and "sidebar".

8. +++ b/core/themes/seven/install-page.css
   @@ -41,147 +73,137 @@ body.install-page {
   +  /* sitename */
   +  .install-page [role="banner"] h1 {
   +    margin: 0.75em 0 0.75em 1.9em; /* LTR */
      }
   

   This comment should be at least capitalized (http://drupal.org/node/1354#drupal) if not fleshed out a bit more.

9. +++ b/core/themes/seven/install-page.css
   @@ -41,147 +73,137 @@ body.install-page {
   +@media all and (min-width: 48em) and (max-width: 975px)  {
   

   Extra space between media query and { can be removed.

I'll make these fixes.

Here's a patch to make the fixes Cottser's suggested in #36.

I kept the same indenting as was in system, although I tend to agree that it makes sense to indent the container div.

Looking much better, thanks @mdrummond!

+++ b/core/modules/system/templates/install-page.html.twig
@@ -12,64 +13,57 @@
+  <header role="banner">
+   {% if site_name or site_slogan %}
+      <div class="name-and-slogan">
+        {% if site_name %}
+          <h1>{{ site_name }}</h1>
+        {% endif %}
+        {% if site_slogan %}
+          <div class="site-slogan">{{ site_slogan }}</div>
+        {% endif %}
+      </div>{# /.name-and-slogan #}
+    {% endif %}
+  </header>

Missed this before, the opening if block needs to be indented by one more space. (Needs to be fixed in the seven template as well.)

Thanks @Cottser! Let's get this in.

updated info about the maintainence template moved to seven theme folder

42: install-page-twig-markup-2041793-42.patch queued for re-testing.

Needed a reroll.

45: install-page-twig-markup-2041793-44.patch queued for re-testing.

After apply the patch the installer looks like the following screenshot:

49: install-page-twig-markup-2041793-49.patch queued for re-testing.

The patch from #49 forks fine form me. I tested it with simplytest.me as well as on my local environment.

It seems testbot is broken now. Some of the servers where updated to PHP 5.3.27 and Debian Wheezy as rfay told me and they are working to fix that.

Also thanks @Dragan. It seems I'm doing something wrong trying to test this patch. I will try again.

Tested again, looks it is really broken.
Will see what I can do here...

Here is new image

I saw it, took screenshot, then I updated issue... But when I returned back to debug and work on it, I just reloaded page and everything works fine again... Tried to remove files, and to create settings.php again, but can't reproduce the BUG again :S

Here is example of showed slogan during installation. looks a bit ugly and like it doesn't follow the layout.

Twig templates are cached by default, so you'd have to clear the cache to see the mark up changes.

Here is example of showed slogan during installation. looks a bit ugly and like it doesn't follow the layout.

There's an issue for that ;) #2043047: Slogan shown on installation pages

New re-roll.

Saw a few things

1. +++ b/core/themes/seven/install-page.css
   @@ -23,170 +20,181 @@
   +}
   

   We are missing some padding here. The previous #page selector was inheriting padding: 20px 0 40px 0; from Seven's style.css before.

2. +++ b/core/themes/seven/install-page.css
   @@ -23,170 +20,181 @@
   +  /* Positioning sidebar & content */
   +  .install-page [role="main"] {
   

   I think we should have a blank line above comments. We also have a mix between block comments and one line comments.

Applied suggested changes in #65

Looks like the the Seven install.html.twig file fell out of the patch Ruben :(

Sorry, hope now it's correct :)

hmm the l-container class is missing a lot of styling. The #page selector still has the width and padding on it.

Alright, I've tested this in all the things. Mobile, desktop, rtl, ltr, tlr. Let get this RTBC'd before we have to reroll again.

Patch applies well and install page looks awesome. Let's go for RTBC!!

Cool, this looks like good clean-up.

Committed and pushed to 8.x. Thanks!

When testing this, I noticed that update.php and install.php are now wildly out of sync with each other in terms of look and feel. Is there a separate issue about cleaning that up?

Please ignore me if this is stupid question but I'm wondering why do we need two install-page.html.twig files (one in /core/themes/seven and one in /core/modules/system/templates)? It seems there's one too many ;)

When testing this, I noticed that update.php and install.php are now wildly out of sync with each other in terms of look and feel. Is there a separate issue about cleaning that up?

I have been thinking about this. The installation design feels about 'loud' for update.php but we could have a tweaked version that's a little calmer. The progress elements should be more consistent at least.

#2169765: Redesign update.php to be more consistent with the installation process

Please ignore me if this is stupid question but I'm wondering why do we need two install-page.html.twig files (one in /core/themes/seven and one in /core/modules/system/templates)? It seems there's one too many ;)

/core/modules/system/templates is the default for every theme used in Drupal, the #1980004: [meta] Creating Dream Markup initiative aims to remove a lot of default Drupal html that is in 'just in case' some one needs it. If a theme requires extra elements or classes then the html should only be loaded for that theme not every theme.

@LewisNyman ok so we have two install-page.html.twig files and that's fine, thanks for clarifying that!

One last thing that bothers me is the /core/themes/seven/install-page.html.twig which should rather be in /core/themes/seven/templates/install-page.html.twig.

Yes, I think install-page.css should probably get moved to the 'css' folder too. Can we get a small follow-up issue created to fix up those items? Thanks!

Edit: Actually there's a bunch of loose CSS, what do you think @LewisNyman? https://drupal.org/node/1887922 seems to say they should live under 'css'. Maybe there's an issue already.

I say this definitely should be cleaned up. While moving the files we might change "seven.base.css" to "base.css" to unify naming too.

Edit: duplicate issue link deleted.

Wr already have an issue to move the CSS into a directory #2033211: Move Seven CSS files into css directory.