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

Wait - we have a standard horizontal bar? This should not have the same background color as the toolbar second menu - that would be confusing.

Button would be nice to fix, including adding a proper <

My take: because we need a unique look here to clearly signal that you're looking at a preview, this should not blend in but stick out a bit instead.

I will just nod at what yoroy said.

Taking it litterally that we want to warn people we could maybe derive a nice yellow from our warning message. I'm reusing the yellow from our warning messages in the attached mockup but it looks a bit weak to me. I simplefied the button because maybe it needs less of an accent when the whole bar gets more noticeable.

I like the way this is going. Maybe we could have a small talk tomorrow about which direction we'd like to go with this. I like the idea proposed in #9.

That looks great @yoroy. Did you decide on anything @sqndr?

We haven't decided on anything at Amsterdam. Here's a patch implementing (more or less) the solution from #9.

I definitely like how much simpler the CSS becomes — this patch mostly deletes code! :)

+++ b/core/themes/bartik/css/style.css
@@ -847,9 +847,7 @@ ul.links {
+  background: #FFFCE5;

Don't we use lowercase hexadecimals in our CSS?

Some fixed lines...

Here is a patch from #19:

fixed :)

Patch does not apply any more.

Trying to review this.
The patch in #22 applies correctly in my environment.

The button is gone, the font looks correct to me, but the underlying bar is blue.

Patch from #22 applied cleanly.

Can confirm button is gone and the bar is blue.

However once the screen width drops to 900px the edit bar sites over the top of the Drupal logo:

Ok, I revisited the patch at #22, and everything seems to be actually ok. The bar is indeed yellow, and I could not replicate the problem pointed out by @dahousecat about the bar going over the logo. Attaching screenshot. (Chrome - mac - 890px).

Marking as reviewed, but feel free to double check.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

What you think of this?

LTR:

RTL:

Updated the issue summary as per #30.

+++ b/core/modules/node/src/Form/NodePreviewForm.php
@@ -83,7 +83,7 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
-      '#options' => array('attributes' => array('class' => array('node-preview-backlink'))) + $query_options,
+      '#options' => $query_options,

Why did you revert this? I felt like it's was a good thing to remove the class?

I revert this for styling the element "Back to content editing" to "< Back to content editing", because I found no way to apply a css style.

I have reviewed the patch in #31. All seems good to me.

Screenshots:
LTR:

RTL:

Why do we have the dotted line under the back link? We dont need that.

It looks like this is the default way of displaying "actionable" links in the bartik theme:

so to me it makes sense, at least?

Not really, we should use Seven's pattern here. Just like the toolbar doesn't follow Bartik. This should not follow Bartik.

and now?

RTL

LTR

Patch #43 looks good. Tested and confirmed that with the patch it looks like the screenshot from Lewis.

Please consider this issue also, https://www.drupal.org/node/2384153

That can be a follow up.

Restoring...

Following this so that I can re-roll the patch in #2384169: The node preview bar is not usable without Bartik once this one is committed.

1. +++ b/core/themes/bartik/css/style.css
   @@ -870,49 +868,32 @@ ul.links {
   +  background: url(../../../misc/icons/0074bd/chevron-left.svg) left no-repeat;
   ...
   +  background: url(../../../misc/icons/000000/chevron-left.svg) left no-repeat;
   

   missing /* LTR */

2. +++ b/core/themes/bartik/css/style.css
   @@ -870,49 +868,32 @@ ul.links {
    .node-preview-backlink:hover {
   ...
   +  text-decoration: underline;
   +  border-bottom-style: none;
   ...
    [dir="rtl"] .node-preview-backlink:hover {
   ...
   +  text-decoration: underline;
   +  border-bottom-style: none;
   

   I don't think that we need the same styles on the rtl only css.

+}
\ No newline at end of file

NOTE: At Drupal Sprint in Austin and looking at this.

Tested patch #55.
Patch applied cleanly
The trailing space issue has been resolved.
Both RTL and LTR displays are a shown in @rpayanm comment #40.

Alex has been most involved here, so kicking back to him.

And the UI is as intended, so we're good on that part too.

Unrelated fail in useradmintest.

+++ b/core/themes/bartik/css/components/node-preview.css
@@ -1,8 +1,6 @@
+  font-family: "Lucida Grande", "Lucida Sans Unicode", "DejaVu Sans", "Lucida Sans", sans-serif;

So... this entangles bartik with seven's font stack. Should we be doing that? This feels problematic for admin themes other than seven. Not sure what to do here. The least we should do is add a comment.

rerolling a patch #55 :)

forget to add SVG icons ;)

I still don't think putting seven's font stack in bartik makes sense since this means that we're saying bartik should be used with seven - but other admin themes are possible.

Removing sprint weekend tag!!
As suggested by @YesCT

Sorry, these issues were actually worked on during the 2015 Global Sprint
Weekend https://groups.drupal.org/node/447258

This has nothign to do with the entity system :)

I don't the problem with the test. It seems there is a wrong theme name parameter with value 0 passed to the function drupal_get_filename() at some point but can't debug it ATM.

Fixed a couple of nits and it looks great. Check the screenshots:

NODE PREVIEW BEFORE

NODE PREVIEW AFTER

1. +++ b/core/modules/node/node.module
   @@ -155,6 +155,48 @@ function node_theme() {
   + * An admin theme can specify CSS files to make the front-end administration
   + * experience of in-place editing match the administration experience in the
   + * back-end.
   

   How is this related to in-place editing?

2. +++ b/core/modules/node/node.module
   @@ -155,6 +155,48 @@ function node_theme() {
   +function node_library_info_alter(&$libraries, $extension) {
   

   This is the exact same logic as quickedit_library_info_alter(), which has been thoroughly analyzed, and which has proven to work well.

3. +++ b/core/modules/node/node.module
   @@ -155,6 +155,48 @@ function node_theme() {
   +            $library['css']['theme']['/' . $theme_path . '/' . $path] = ['weight' => 20 ];
   

   This weight is the only change compared to quickedit_library_info_alter(). If we really need it, we should document why it's necessary.

4. +++ b/core/modules/node/node.module
   @@ -155,6 +155,48 @@ function node_theme() {
   +}
   +
   +
   +/**
   

   Nit: two \ns, should be one.

There are no longer any traces of the node preview bar in Bartik.

I noticed a few visual issues at mobile widths.

The dropdown section and back link do not sit next to each other at small widths and it looks messy - noticed on an iPhone 5.
 

 
With Deutsch at around 400px.
 

 
Also when testing in Chrome, the toolbar would appear over the preview bar if you scrolled through the content and then went back to the top.
 

 
However in iOS this wasn't the case and the preview bar left a toolbar sized gap above it no matter how much you scrolled.
 

 

Reroll

The node preview styles no longer exist or referenced within Bartik ✔︎
Node preview now belongs to Seven and loads the files correctly ✔︎

Here are before and after screenshots.

Before

 
After

The node preview bar now has a yellow background to warn the user that the content is just a preview ✔︎
The overall design has been tidied up and I feel matches the Seven theme + style guide well ✔︎

I approve — RTBC!

strange test failure, I am re-queueing and will take a quick look at the test.

I can recreate the test error failure locally, and the same error does not occur for me without your patch (althought I do get a different error.) I wonder if your closure trick is tripping out PHP?

Corroboration would be helpful. Can someone else run the tests locally after applying the patch.

The fail started showing up after the hook added in #76. I can try and help but I do not know how to set up tests locally

error: patch failed: core/themes/seven/seven.info.yml:13

Tests which failed before, are not failing now.
I have a bit problems with testing, but i think it is just problems with my virtual box.

+++ /dev/null
--- a/core/themes/seven/seven.info.yml
+++ b/core/themes/seven/seven.info.yml

+++ b/core/themes/seven/seven.info.yml
+++ b/core/themes/seven/seven.info.yml
@@ -10,8 +10,8 @@ libraries:

@@ -10,8 +10,8 @@ libraries:
  - seven/global-styling
 stylesheets-remove:
   - core/assets/vendor/jquery.ui/themes/base/dialog.css
-quickedit_stylesheets:
-  - css/components/quickedit.css
+node_preview_stylesheets:
+  - css/components/node-preview.css
 regions:
   header: 'Header'
   pre_content: 'Pre-content'
 

The patch is failing because you accidentally removed the quick edit stylesheet, the rest is fine :-)

@emma.maria I put back quick edit stylesheets

I reviewed the PHP and it looks good for me.

Now that we have had a visual and a code review, let's do this!

*throws RTBC confetti in the air*

Ok, I put back in those files.

tests passed, RTBC by @lewisnyman.

T

glitchy bot.. Not really RTBC'ing my own patch, really...

This should be done somewhere else than in Seven per. discussion that was had in DrupalCon Barcelona.

Hmm, lets have Lewis chime in here yet - because we have don't really have a decision there yet.

Re-roll.

+++ b/core/modules/node/node.module
@@ -155,6 +155,47 @@ function node_theme() {
+  if ($extension === 'node' && isset($libraries['drupal.node.preview']) && Drupal::config('system.theme')->get('admin') != '0' ) {

This != '0' is really weird. Where in ConfigTranslationUiTest does it do this - I couldn't spot it.

+++ b/core/modules/node/node.module
@@ -155,6 +155,47 @@ function node_theme() {
+            // Ensure it overrides any styling from the frontend theme.

It doesn't override any CSS from frontend theme, its loaded where module CSS which is before theme CSS. Either there is something wrong in the logic or the comment is wrong. Anyway do we want to give all the power for the admin theme? What about the case of having dark and light themes? What if those themes are dynamic?

Commented a bit longer one here #2566775-8: [Voltron patch] Move all remaining *.admin.theme.css to Seven

It would be nice to have a screenshot here.

Thank you @LewisNyman

It would be nice to see test coverage that the CSS files are being added and maybe additionally if its simple enough for the order too. I think its also worth at least a manual test (automated test preferred) to ensure it works with base themes too.

@lauriii:

It would be nice to see test coverage that the CSS files are being added and maybe additionally if its simple enough for the order too. I think its also worth at least a manual test (automated test preferred) to ensure it works with base themes too.

Note that quickedit.module has an identical feature: it allows admin themes to specify quickedit_stylesheets. Seven uses that to make in-place editing on the front-end be consistent with the back-end. See quickedit_library_info_alter(). To my great shame, there apparently is no test coverage for that. It was committed as part of #1824500: In-place editing for Fields in December 2012, and has not been broken once. But, it really should have test coverage. It slipped through the review cracks back then, and was forgotten until now.

So, I think it makes more sense to not hold this issue back on those tests, because Quick Edit should also get those tests. Furthermore, even if this were broken in some subtle way that all the manual testing so far has not yet uncovered: then it'd only affect pages adding the node/drupal.node.preview asset library. So the damage would be extremely isolated.

I think it's important this follows the same pattern as Quick Edit: just like Quick Edit shows some UI bits that should match the back-end's theme, so should this.

Moving back to RTBC.

One more thing that should be considered before committing the fix as is the admin theme permissions that this patch might have to check.

Thanks @LewisNyman! Can we add a more detailed explanation of why that's the case to the summary (e.g. <h3>Why this change should be committed during RC</h3>)? Also include any disruptions from the change.

Updated per #148. The fail seems weird, rerunning tests.

Giving a stronger title; "tweak" sounds like polish as opposed to a usability issue. Thanks!

Can someone clarify whether the bugs @emma.maria describes in #92 are fixed by this patch, or are out-of-scope bugs that will still need to be fixed later in #2524284: The spacing of the buttons in the preview bar is cramped on narrow screens?

I would feel a lot more comfortable evaluating this patch for RC target triage if the fix to make it work on mobile/not-Bartik were decoupled from the design changes. The former is a user-facing bug, and very easy to justify committing during RC, the latter feels like more of a minor version target at this point.

If for some reason they can't be split apart, the rationale for that would be good to understand.

Rerolled patch and fix warnings

Rerolled and added Screenshots

So the idea was, node preview bar not get hide any content but with the latest patch 'User account menu' is getting hidden. Screenshot attached.

FYI. I have disabled the background color of preview bar so that i can check the content.

Ok, i addressed the comment from #167

This probably needs another redesign now that Claro is in core?

It may already be entirely different in Claro, needs to be checked.

It's not, but it might be different in *Olivero*, because this really is about the frontend them, the backend theme currently has no control over it.

Tested quickly. Olivero is actually pretty decent I think, umami is completely broken. The approach here is about ensuring a consistent look no matter the frontend theme you use.

That would be the realm of #2195695: Admin UIs on the front-end are difficult to theme though then as a concept?

The issue summary is referencing many no-longer-in-Drupal things. The summary needs to be updated to make this coherent.

I'm tempted to set this to closed (Outdated) as I'm not sure how beneficial the prior 189 comments would be to addressing any issues with this in Claro, etc, but I'll let someone else who has been more involved in that issue make that call more conclusively.

It has been1 year and 3 months since @bnjmnm suggested closing this issue for reasons explained in their comment, #190. Since no one who has worked on this issue before has responded I think it is time to close this.