The spacing of the buttons in the preview bar is cramped on narrow screens

Added spacing to prevent overlapping of select box in preview bar for ltr and rtl

I have rewritten the node-previrew.css using existing min-width media query.

Patch looks good to me. Attaching a Shot.

The screenshots in #1 made me realize something, having recently debugged the Toolbar code.

The real problem here is that the preview bar simply sits on top of the page. It instead should be pushing the rest of the page down, just like the Toolbar does. Attached is a screenshot that clearly illustrates it.

I know that none of this uses any JS (this is generated by \Drupal\node\Form\NodePreviewForm::buildForm()). If this were not using CSS, but instead used the Drupal.displace() API, this problem would not exist.

Drupal.displace() was created as part of the Toolbar work, but it's already a generic component that anything that wants to basically "change the viewport" — which this preview bar is one example of, the Toolbar is another — can and should use. See drupal.displace in core.libraries.yml. Sorry for not making that more clear!

So, given that, AFAICT this really is exactly the issue reported here? If the preview bar started to use Drupal.displace(), all of the changes here would basically be reverted AFAICT. If I'm right about that, then it feels like we should just apply the proper solution right away here. But, if you feel it's better to get this in as an interim step, that's fine by me too. As long as you're aware that this is not a real solution.

Oh! I'm very sorry. I read "cramped on narrow screen" and saw the screenshot in #8. I totally missed the screenshot in the IS.

Apologies!

Filed #2550691: Node preview bar occludes page header, should use Drupal.displace() JS API instead of CSS for what I thought we were discussing.

1. +++ b/core/modules/node/css/node.preview.css
   @@ -11,11 +11,34 @@
   +[dir="rtl"] .node-preview-container .node-preview-backlink {
   +  float: none;
   +}
   +
   +[dir="rtl"] .node-preview-container .form-type-select {
   +  display: block;
   +}
   

   Why are these changes necessary?

2. +++ b/core/modules/node/css/node.preview.css
   @@ -11,11 +11,34 @@
   +  .node-preview-container .node-preview-backlink {
   +    float: left;
   +  }
   +  ¶
   

   There is unneeded spaces on the last line here. And the float should have /* LTR */ after it.

I am working on this at the dug-BE code sprint in Ghent.

Tag fix

Referring to comment 14
1. Float none and display block were not needed. Please see attached screenshots for before and after changes. The code was deleted.
2. I removed the two superfluous spaces and added the LTR comment.

Please review.

This doesn't really look OK. See screenshot.

+++ b/core/modules/node/css/node.preview.css
@@ -15,19 +15,11 @@
-[dir="rtl"] .node-preview-container .form-type-select {
-  display: block;
-}

I do believe we need this part. (I added the CSS back to in Chrome to check this. See attachment 2)

According to @pjbaert

Attached patch.

I tested the issue.
Issue is still reproducible with the patch.

before patch

after patch

Meanwhile unassigned issue i am not a UI expert.

Fixed this issue. Added display block for that select div. Related to the node-preview-class.

With #2341221: Node preview bar has usability issues, is difficult to use on mobile, not usable without Bartik, and does not align with the Seven style guide and current toolbar designs I am unable to replicate this? Or is this specific to one platform?

This is specific to bartik

Tested the_spacing_of_the-2524284-24.patch on Firefox & chrome browser for Ubuntu 14.0.4.
Spacing added to prevent overlapping of select box in preview bar.
Patch looks good for ltr. But for rtl the issue is not resolved after applying the patch.
Screenshots attached.

The latest patch contains unrelated changes in sites/default/ directory.

@rashid can we please have interdiff please

#29 patched fixed the issue, but created a new issue for wide screens. At this point I didn't continue to check RTL. Screenshots below.

EDIT - New post to follow. I missed some RTL stuff with the 'back to content editing button'

Created a new patch to fix this issue. All changes made to core/themes/bartik/css/components/node-preview.css

Modifications include:

.node-preview-container
padding: 10px; changed to padding: 5px 10px;

.node-preview-backlink
margin: 0; changed to margin: 5px 10px 5px 0; /* LTR */
Added display: inline-block;

[dir="rtl"] .node-preview-backlink
Added margin: 5px 0 5px 10px;

Added

[dir="rtl"] .node-preview-backlink::after {
  content: '';
  width: 10px;
  display: inline-block;
}
.node-preview-container .form-item-view-mode {
  display: inline-block;
  margin: 5px 0;
}
[dir="rtl"] .node-preview-container .form-item-view-mode {
  float: right;
}

------------------------
Before and Afters

Right to Left
NOTE: I couldn't figure out how to simulate this in my browsers. This is just showing the RTL code applied manually simulating the best I could.

Patch worked nicely for me. I also tried it out RTL and it appears appropriate- slightly different from the simulated version above because of where the field label and colon appears, but the styling itself is right. Steps for anyone else to confirm there as well:

• Enable Language module.
• Go to Configuration > Regional and language > Language.
• Add the Arabic language. Save.
• Set Arabic as default.
• View the page and preview as usual - still appears in English.

Also manually testing

Also tested and can confirm using same steps as @webbykat outlined.

Also tested and confirmed as Webbykat had outlined in #36.

Did see another styling issue, though--the dropdown select reverts to defaults in Firefox. This is shown in the screen grab:

This is specific to Firefox, as shown on the same screen in Chrome:

I believe that the FF has a different CSS selector for that select element. I'll look into it and see what I can find.

So, the default styling for the dropdown/select is created by the user agent stylesheet. Do we need the Firefox select to look the same as the Chrome, and if so, should we be checking on other browsers to try and match the styles?

Moving to Bartik.

Knocking this down to Needs Review as there are some concerns in the last few comments.

There's a better looking select list in Firefox when using Drupal 8.0 so looks like we lost some styling indeed.
The same select already looks worse (default) when using Firefox and Drupal 8.1.x *without this patch*, so this little visual regression is probably not caused by this patch.

@yoroy, I just dig into the commits, it seems like the nice select styles for Firefox appears in Seven theme only, not ported into Bartik yet.

Commit 9e6dd21
core/themes/seven/css/style.css

 * Select elements - Webkit only
 */
@media screen and (-webkit-min-device-pixel-ratio: 0) {
  select {
    cursor: pointer;
    -webkit-appearance: none;
    padding: 1px 1.571em 1px 0.5em; /* LTR */
    border: 1px solid #a6a6a6;
    border-radius: 0.143em;
    background:
      url("../../../misc/icons/333333/caret-down.svg") no-repeat 99% 63%,
      -webkit-linear-gradient(top, #f6f6f3, #e7e7df); /* LTR */
    text-shadow: 0 1px hsla(0, 0%, 100%, 0.6);
    font-size: 0.875rem;
    transition: all 0.1s;
    -webkit-font-smoothing: antialiased;
  }
  [dir="rtl"] select {
    padding: 1px 0.714em 1px 1.571em;
    background-position: 1% 63%, 0 0;
  }
  select:focus,
  select:hover {
    background-image: url("../../../misc/icons/333333/caret-down.svg"),
    -webkit-linear-gradient(top, #fcfcfa, #e9e9dd);
    color: #1a1a1a;
  }
  select:hover {
    box-shadow: 0 1px 2px hsla(0, 0%, 0%, 0.125);
  }
}

/**

I am going to jump in here and say that the "look and feel" styling of the components themselves within the preview bar is out of scope for this issue.
As long as the horizontal layout of the components is no longer broken this issue is complete and we can most definitely raise a follow up for the select element styling in Bartik.

Agreed that whatever needs to be done to improve the look of the select list is not in scope of this issue.

Thanks for digging that up @ironkiat. Could you create a seperate issue for that in the Bartik queue?

Patch in #24 still worked just fine on simplytest and Drupal 8.1, so probably rtbc

Thanks @yoroy, created issue: https://www.drupal.org/node/2674292

#24 would need work (it incorrectly changes files outside of the scope of this issue). #34 seems more promising overall. Although it's more complex it seems like it had a lot more testing than #24.

Putting to needs review so we can decide which fix we want to go with and give the relevant patch another round of manual testing and screenshots please :)

Tested and reviewed patch 34, seems to work pretty well on both desktop and mobile sizes.

Desktop:

Mobile:

On Firefox, after applying the patch in here: https://www.drupal.org/node/2674292 - the select element looks as per the screenshot above.

A possible further fine tuning may be to float the entire "View mode" to the right and when screen sizes gets really small (below 320px) then we remove the float.

I'm going to do the changes that #53 said.

Ready to review!

Looks good

#55: Could you provide an interdiff? https://www.drupal.org/documentation/git/interdiff

There's a problem with the view mode select list becoming invisible with the toolbar in vertical mode. See attached screencast

@ironkiat Why do you think it is an improvement to float the view mode select list to the right? For left-to-right languages this drastically reduces the discoverability which I'm not sure we want to do.

Speaking of left-to-right, maybe this needs to be checked with a right-to-left language as well.

Interdiff

@yoroy, totally agree, yeah I'm not too familiar how it should be when it's for right-to-left language...does the Back to Content Editing appear on the right instead?

I suggested the view mode select list to be on the right because I felt it should be separated from the "back button". Giving it some visual separation will possibly help to not let users feel they belong to the same group / action, which they shouldn't be. Both are very different actions we all know that.

Tested the latest patch #55 for both LTR and RTL directions and I agree with @yoroy. By floating the view mode select list to the right in LTR, or to the left in RTL pushes the view mode out and it becomes invisible (when the toolbar is in vertical mode). Attaching SS.
As per my views, we should keep the select list aligned with the "back to content editing" button for desktop, and for mobile layout select list will appear just below the button as done in patch #34.

Applied patch #34 - https://www.drupal.org/files/issues/preview_bar_button_spacing-252484-34..., LTR looks good.
However, for RTL there is no change. And for screen resolution 768px view mode select list is cropped.
Adding the Screenshots for RTL.

Added a patch to fix issue for RTL.

i'm working on this

I had applied the patch and tested it locally. It looks fine to me

Here are the screenshots of the page before applying the patch

Here are the screenshots of page before the fix

I have tested it manually and looks fine to me. Here are the screenshots of before and after the patch updates:

AFTER
https://www.drupal.org/files/issues/after-patch-mobile-RTL.png
https://www.drupal.org/files/issues/after-patch-tab-RTL.png
https://www.drupal.org/files/issues/after-patch-desktop-RTL.png
https://www.drupal.org/files/issues/after-mobile.png
https://www.drupal.org/files/issues/after-tab.png
https://www.drupal.org/files/issues/after-desktop.png

BEFORE
https://www.drupal.org/files/issues/before-patch-mobile-RTL.png
https://www.drupal.org/files/issues/before-patch-tab-RTL.png
https://www.drupal.org/files/issues/before-patch-desktop-RTL.png
https://www.drupal.org/files/issues/before-patch-mobile.png
https://www.drupal.org/files/issues/before-patch-tab.png
https://www.drupal.org/files/issues/before-patch-desktop.png

I have tested it manually and looks fine to me. Here are the screenshots of before and after the patch updates:

AFTER

RTL - Mobile:

LTR - Mobile:

RTL - Tab:

LTR - Tab:

RTL - Desktop:

LTR - Desktop:

BEFORE

RTL - Mobile:

LTR - Mobile :

RTL - Tab:

LTR - Tab:

RTL - Desktop:

LTR - Desktop:

i have further validated this issue at drupal camp delhi 2016 . please refer attached screenshot.

RTL BEFORE

RTL AFTER

LTR AFTER

LTR BEFORE

Thanks for all the work, excited to see this go in!

Updating the summary to link the manual testing in #72 rather than every screenhsot, because every time I look at this issue, I am completely overwhelmed and can't figure out what I am looking at.

Couple things I noticed:

1. +++ b/core/modules/node/css/node.preview.css
   @@ -11,11 +11,34 @@
   +[dir="rtl"] .node-preview-container .node-preview-backlink {
   +  float: none;
   +}
   ...
   +
   +  [dir="rtl"] .node-preview-container .node-preview-backlink {
   +    float: right;
   +  }
   

   Isn't the second hunk here overriding the first? Let's remove the first one? Edit: Ah, I see, the second is only for the media query version. Still, the first hunk looks incorrect and/or unnecessary, since we never specify this for LTR.

2. +++ b/core/modules/node/css/node.preview.css
   @@ -11,11 +11,34 @@
   +  .node-preview-container .node-preview-backlink {
   +    float: left;
   +  }
   +  ¶
   

   There is trailing whitespace at the end of this hunk. We should reroll to fix that, at least.

Thanks everyone!

+++ b/core/modules/node/css/node.preview.css
@@ -11,11 +11,34 @@
+.node-preview-container .form-type-select {
+  display: block;
+}
...
+[dir="rtl"] .node-preview-container .form-type-select {
+  display: block;
+}

Should these be combined into a single entry for clarity?

Hi,

the code does not match with the code at #81 and #82, and patches updated, in branch 8.4.x.

I've just removed the space in line 23 of node.preview.css at core/modules/node/css.

#85 patch apply cleanly. Not seen any visualize ui impact before and after apply patch. Observe some code level changes happen in the core/modules/node/css/node.preview.css and core/themes/bartik/css/components/node-preview.css

In the node/css/node.preview.css file just remove new line space and in bartik/css/components/node-preview.css file apply some css changes or patch add these new css:

[dir="rtl"] .node-preview-backlink::after {
  content: '';
  width: 10px;
  display: inline-block;
}
.node-preview-container .form-item-view-mode {
  display: inline-block;
  margin: 5px 0;
}
[dir="rtl"] .node-preview-container .form-item-view-mode {
  margin-right: 0;
}

Attached the some before and after screenshot.

We (leanderl, sannastrand, marikaj) are reviewing this on DrupalCon Vienna...

Applied and tested patch in #65. It works very well for LTR and RTL.

The discussion in #81 and #82 seems to be about some patch differerent than #65. For that reason we have rerolled, tested and reuploaded the patch in #65.

Set to RTBC like it was in #74 and further confirmed in #75.

Worked on this on Vienna 2017 Code sprint with
marikaj, sannastrand

got mentorship from jcventura

I removed changes made to the core CSS files since they were unrelated for the actual bug fix. Other than that the changes look good visually and code wise.

Committed 4a1d43f and pushed to 8.5.x. Thanks!

I haven't cherry-picked this to 8.4.x as I will still check with the other maintainers if that's fine.

Discussed this issue with @xjm and we decided not to backport this to 8.4.x. 8.4.0 was released, so we think that is better to leave this just for the next minor release. It takes a lot of energy to make assumptions of how disruptive some CSS changes are, so I don't think it is worth taking a risk on this issue. As mention earlier, this has been already committed to 8.5.x, and will be released on 8.5.0. Thanks everyone for your contributions!