Posting patches as I go; it's a work in progress.

Changes

(Each round builds upon the previous round.)

  1. Round 4 (#30):
    • Use the hand cursor when hovering an editable field.
    • Fix toolbar pointer's background color.
  2. Round 3 (#26):
    • Only transition the opacity property on the buttons, not everything, because that caused glitches with the new CSS.
    • Apply Bojhan's changes in such a way that Seven-specific changes now live in the Seven theme.
  3. Round 2 (#16):
    • Make it possible for the admin theme to override Edit entity toolbar's default styling.
    • Add blue pencil icon to entity toolbar, and stop bolding the field label.
    • Refine field 'outlines' (box shadows, really).
    • Reintroduced hover state for the close button, as well as a 'pressed state'.
  4. Round 1 (#2):
    • Adjust the style of in-place editing CKEditor instances.
    • Updated entity toolbar labeling.
    • Make the toolbar much lighter.
    • Fix slight horizontal movement of entity toolbar for padded textual fields.
    • Replace the close icon with a Libricon SVG equivalent.
    • Fix 'doubled' fields: instead of an outline around the field itself *and* around the form in-place editor "pop-up", only keep it around the currently interacted element.

Reference screenshots

Reference screenshot #1
Reference screenshot #2

Polish round #1 screenshots

Reference screenshot #1
Reference screenshot #2

Polish round #2 screenshots

Reference screenshot #1
Reference screenshot #2
New hover/pressed styling for the "close" button
close-hover.png
close-press.png

Polish round #3 screenshots

Reference screenshot #1
Reference screenshot #2

Polish round #4 screenshots

Reference screenshot #1
Reference screenshot #2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

FileSize
51.86 KB
49.07 KB

Reference screenshots for in the issue summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
16.53 KB
45.71 KB
50.08 KB

Patch attached for round 1. Uses git format-patch so you can assess the individual improvements.

Improvements in this round:

  • Adjust the style of in-place editing CKEditor instances.
  • Updated entity toolbar labeling.
  • Make the toolbar much lighter.
  • Fix slight horizontal movement of entity toolbar for padded textual fields.
  • Replace the close icon with a Libricon SVG equivalent.
  • Fix 'doubled' fields: instead of an outline around the field itself *and* around the form in-place editor "pop-up", only keep it around the currently interacted element.

Screenshots: see attachments, also in the issue summary.

Wim Leers’s picture

Issue summary: View changes

Insert reference screenshots.

aspilicious’s picture

Not a themer at all. So I could be wrong :)

+++ b/core/modules/edit/css/edit.icons.cssundefined
@@ -51,14 +51,12 @@
+  background-image: url('../../../misc/icons/787878/ex.svg');

Not an expert but will ther e be a fallback for png? If we want to tweak drupal 8 for our client to support ie8 in the frontend we need some kind of fallback icon.

+++ b/core/modules/edit/css/edit.theme.cssundefined
@@ -162,11 +162,16 @@
+  content: ' → ';

Does this work for all the browsers we support?

+++ b/core/modules/edit/js/theme.jsundefined
@@ -54,7 +54,7 @@ Drupal.theme.editEntityToolbar = function (settings) {
+  return '<b class="field">' + settings.fieldLabel + '</b>' + settings.entityLabel;

Isn't better to use a span with a css class to apply the boldness.

+++ b/core/modules/ckeditor/css/ckeditor.cssundefined
@@ -23,3 +23,17 @@
+  border-top: none;
+  border-right: none;
+  border-bottom: none;
+  border-left: none;
+  box-shadow: none;

whats wrong with "border: none"

Wim Leers’s picture

Status: Needs review » Needs work

Okay I should've been far more explicit: this is not yet ready for review!

A good review, but just too soon :) I'll address it as soon as it is complete.

Wim Leers’s picture

aspilicious’s picture

One small thingie that annoys me but I'm not sure if it can be fixed. When hover field_tags you get the nice not to hard border color. When you click you get a popup with another border.
==> 2 boxes with borders fighting each other.

Expected behaviour:
Only put focus on the top layer. I'm not interested in all the other layers and they distract me.

Wim Leers’s picture

#6: yes, that's fixed already in #2 :) ;)

Fix 'doubled' fields: instead of an outline around the field itself *and* around the form in-place editor "pop-up", only keep it around the currently interacted element.

You can also see this in reference screenshot #1! :)

tkoleary’s picture

@Wim Leers

Looking good. :)

aspilicious’s picture

I also mean all the other borders from the other elements. They aren't of any use anymore. When you save or close you need to click quick edit again.

Wim Leers’s picture

#9: No. The whole point of #1678002: Edit should provide a usable entity-level toolbar for saving fields is that you can jump from in-place editing one field to another field *without* having to click the "Save" button anymore after changing every field. And to let the user know which other fields he can edit, the outlines around other fields must continue to exist even while editing a field.

aspilicious’s picture

Aha interesting! :)

EDIT: although that leads to strange situations:
I quick edit the tag field. I change a value but don't change. I switch to the body. I change some text. I press save.
Both tags and body are changed.
When refreshing, tags are reverted.

Just a note...

Wim Leers’s picture

#11: if you find bugs, please create a new issue. This issue has now been sidetracked a lot already ;)

tkoleary’s picture

@Bojhan

You asked for the invision design which I thought I had already posted. It's here: http://invis.io/QBH9N85K

Changes mainly clean up some heavy-handed styling and bring it in to sync with seven style guide.

tkoleary’s picture

@Bojhan

Re: seven style. Specifically the button colors, gradients background, fonts and icons.

Bojhan’s picture

@Kevin That looks great, happy to see the style guide integrated like that! I have a few small suggestions, I will try to mock them up tomorrow :)

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
439 bytes
405 bytes
50.75 KB
46.57 KB
24.87 KB

Round 2.

Patch attached for round 2. Uses git format-patch so you can assess the individual improvements.

New improvements in this round (on top of the improvements in round 1, see #2):

  • Make it possible for the admin theme to override Edit entity toolbar's default styling.
  • Add blue pencil icon to entity toolbar, and stop bolding the field label.
  • Refine field 'outlines' (box shadows, really).
  • Reintroduced hover state for the close button, as well as a 'pressed state'.

Screenshots: see attachments, also in the issue summary.


#3:

PNG fallback?
Good question :)
In-place editing requires contentEditable, which does not work reliably in <=IE8/Android 2.x. I.e. wherever in-place editing can work, SVG is supported. Finally, we only support IE8 for end-user browsing, not for administration, let alone for advanced features like in-place editing.
Does this work for all the browsers we support?
Yes :)
Isn't better to use a span with a css class to apply the boldness.
That was rather crappy indeed. It's gone now.
whats wrong with "border: none"
Good question again! Using just border: none is what I tried first. Then it turned out this did not have the desired effect — if some other CSS defines border-left: SOMETHING, then setting border: none won't override that. Hence I had to list all of them :(

I think that means your concerns are now answered?

Wim Leers’s picture

Issue summary: View changes

Insert polish round #1 reference screenshots.

Bojhan’s picture

Ok, lets see how many mistakes I made. I spend about 2 hours on this, mostly minor tweaks - nothing fancy. Below is a list of changes:

1) Added border radius (4px). I wasn't sure why all the corners where strong, the style guide specifies more softer corners. Given that the actual CKEditor buttons do this as well. I added the correct border radius to buttons.

2) Border color : #BDBDBD. The color that is used for the current border is too strong, making it stand out too much. I added the style guide specified border color also used in VT's and other containment elements.

3) Adjusted font size. It had a "hard" font size, I have no idea why this was - and it was quite big compared to all other administrative items. I aligned it with the toolbar size and simply threw away the definition.

4) Size of pencil icon. When the text changes the pencil icon also needed to change in size, this was because it would otherwise feel too big compared to the text.

5) Save button. This looked kinda like a hybrid, it used a similar shade to the style guide but not exact. I simply copied over the style guide CSS, and this also adds a significantly more noticeable hover style. The "clicked" style also seemed very "special", I haven't seen this elsewhere in core and you can still click it so I removed it.

6) Exit button. There is no standard for this yet, but the transparent background looked very odd to me. Its also not a style we use anywhere else in core. Therefor I went with just accentuating the button using the Drupal Blue. I tried the selection color but that looked rather weird.

7) Highlight colour for editable fields. I never liked the fact that we are using two shades here. It made it more "busy" than needed. Therefor I removed the second styling, from my point of view this makes the border less busy. I did this for focused and error too.

During testing I noticed a few bugs (not all of them directly related to Edit inline), Wim already told me a few of these are on his planning to open:

- The throbber is really ugly.
- The source option doesn't open in a core dialog.
- The "Loading" indicator when you add an image occasionally popsup at weird places (e.g. behind the entity toolbar)
- The initial position and width of the Entity toolbar is rather distracting (Wim will open an issue about this, once this polish work is done)
- It's often somewhat confusing how to "trigger" the inline mode, perhaps we can change hovering over a field into a hand to serve as indicator.

entity-toolbar-after-review.png

tkoleary’s picture

@Bojhan

I'm ok with all of that except the border.

The reason for the hard border is that, unlike anything else in the style guide, this floats on top of many different types of content of any color or background. The border from the styleguide always sits on white.

Without a stronger border the toolbar just fades into the content:

image

tkoleary’s picture

@Bojhan

Also, in your patch the icons are not loading as you can see.

Bojhan’s picture

@tkoleary Sure, though I am not sure it makes much of a difference when you are floating above text like that. I have experienced that moving around with the inline editing as really jarring - because it jumps around the whole time.

tkoleary’s picture

@bojhan

it jumps around the whole time.

Jesse Beach is working on a fix for that

Bojhan’s picture

Cool :) Well looking forward to that then.

nod_’s picture

Issue tags: +JavaScript

tag.

Wim Leers’s picture

Status: Needs review » Needs work

I'm on this, will reroll tomorrow.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
46.5 KB
46.83 KB
32.1 KB

In #16, I added this:

Make it possible for the admin theme to override Edit entity toolbar's default styling.

Bojhan's changes in #18 are about making Edit's default styling match the Seven theme. But really, all the "Sevenisms" belong in the Seven theme, because a different admin theme may be installed. So I moved all of Bojhan's changes in #18 over to core/themes/seven/edit.css if they were Seven-specific. See below.


1) Added border radius (4px). I wasn't sure why all the corners where strong, the style guide specifies more softer corners. Given that the actual CKEditor buttons do this as well. I added the correct border radius to buttons.

This is clearly Seven-specific, so moved this to Seven. I also removed 2 useless border-radius declarations that you added.

3) Adjusted font size. It had a "hard" font size, I have no idea why this was - and it was quite big compared to all other administrative items. I aligned it with the toolbar size and simply threw away the definition.
4) Size of pencil icon. When the text changes the pencil icon also needed to change in size, this was because it would otherwise feel too big compared to the text.

How is 1.1em a "hard" font size? If everybody thinks the smaller size is fine, I'm fine with keeping this adjustment of course. This is not Seven-specific.

5) Save button. This looked kinda like a hybrid, it used a similar shade to the style guide but not exact. I simply copied over the style guide CSS, and this also adds a significantly more noticeable hover style. The "clicked" style also seemed very "special", I haven't seen this elsewhere in core and you can still click it so I removed it.

I think the button looks out of place in general and it definitely feels to dark. I talked to Bojhan and he said it should be like the styles being introduced at #1986074: Buttons style update, minus the extreme roundedness there. He apparently didn't copy it all CSS declarations from there, because after doing that, it now looks much better.
Of course, moved into Seven, because this is highly Seven-specific.

6) Exit button. There is no standard for this yet, but the transparent background looked very odd to me. Its also not a style we use anywhere else in core. Therefor I went with just accentuating the button using the Drupal Blue. I tried the selection color but that looked rather weird.

Sadly, Drupal Blue on the close button conflicts with the Save button above. It's not unlike combining red and orange. Bojhan said in chat he'd look into this. For now, I'm not changing anything about this: it's the same as Bojhan's patch in #18, i.e. a black close icon upon hover. And I honestly think that's fine to keep. Furthermore, the 2px top margin on the save button also shifts this button downwards, making it positioned far too low. Fixed that.
Also Seven-specific of course.

2) Border color : #BDBDBD. The color that is used for the current border is too strong, making it stand out too much. I added the style guide specified border color also used in VT's and other containment elements.

7) Highlight colour for editable fields. I never liked the fact that we are using two shades here. It made it more "busy" than needed. Therefor I removed the second styling, from my point of view this makes the border less busy. I did this for focused and error too.

I share your desire for simplicity. But both of these are problematic on various backgrounds, including dark backgrounds. On Bartik, just change the background color of #main-content to darkgray or to see what I mean. So for now I reverted this, until that concern is answered in your simplified version.


Round 3.

Patch attached for round 3. Uses git format-patch so you can assess the individual improvements.

New improvements in this round (on top of the improvements in round 1 & 2, see #2 and #16):

  • Only transition the opacity property on the buttons, not everything, because that caused glitches with the new CSS.
  • Apply Bojhan's changes in such a way that Seven-specific changes now live in the Seven theme.

Screenshots: see attachments, also in the issue summary.

Wim Leers’s picture

Issue summary: View changes

Updated for round 2.

Wim Leers’s picture

I forgot to answer these:

- The throbber is really ugly.

Agreed, we should improve that. We can do that in this issue.

- The source option doesn't open in a core dialog.

And that won't change. But we should make the styling match: #2090937: Seven theme: style CKEditor-native dialogs to match Drupal-native dialogs.

- The "Loading" indicator when you add an image occasionally popsup at weird places (e.g. behind the entity toolbar)

I haven't seen this happen yet. That sounds like a bug. When you find the steps to reproduce this, please create an issue.

- The initial position and width of the Entity toolbar is rather distracting (Wim will open an issue about this, once this polish work is done)

Issue opened: #2090945: Polish entity toolbar's positioning .

- It's often somewhat confusing how to "trigger" the inline mode, perhaps we can change hovering over a field into a hand to serve as indicator.

So it's the cursor that should change upon hover. That's easy to fix. You can change that here too. That'd not be Seven-specific of course.

Bojhan’s picture

Ok, reviewing this. Overall the improvements look great :) I am happy to see all the parts getting such detailed feedback, this makes me happy and makes it so much easier to provide these kind of reviews. Some nitpicks that I could find:

1) The pointer doesn't continue the gradient, causing it to look somewhat weird (like you just cleaned it ^^). Perhaps the gradient can continue.

2) Lets turn the cursor into a hand, this probably needs you to use it for a short while to know if it feels right.

3) I asked r5yn to provide us with a new throbber icon.

4) Lets leave the exit icon as is for now, I'd like to use our Drupal Blue or Highlight color - but it needs more tweaking/playing with gradients. I tried to do this, but its a bit tricky.

5) The dark side, of this patch. The highlight color, I agree that in dark themes this is suboptimal. However I don't think its much more suboptimal than the way contextual links look in dark themes. Currently it looks bad on both light and dark themes, I am tempted to optimize for one (like we did with contextual links) rather than making a suboptimal look for both. The ideal solution would be for us to provide two styles, one for dark themes and one for light themes and people can just easily implement one or the other (other theme variations, frankly just require a custom style). I'd imagine that is follow up, so for now I'd like to improve our light version and remove the second color. I am open for discussion on this one though, I know what I am proposing here is quite a big followup.

Great to see followups for all the other issues, we identified!

Wim Leers’s picture

I am happy to see all the parts getting such detailed feedback, this makes me happy and makes it so much easier to provide these kind of reviews.

:)

1) The pointer doesn't continue the gradient, causing it to look somewhat weird (like you just cleaned it ^^). Perhaps the gradient can continue.

The only pointer I know is the mouse cursor. In other words: I have no idea what you're talking about :P I didn't touch any of the background gradients AFAIK. Can you explain this a bit more?

2) Lets turn the cursor into a hand, this probably needs you to use it for a short while to know if it feels right.
5) […] I'd imagine that is follow up, so for now I'd like to improve our light version and remove the second color. […]

So these are the two changes that I understand how to make.

When these 3 points are addressed, this patch is ready to go in your mind?

Wim Leers’s picture

Bojhan explained point 1 in chat, it's now fixed. Point 2 is also fixed.

Point 3 is problematic: the proposed single color outlines don't distinguish between a field that can be in-place edited but is not highlighted versus one that is highlighted (i.e. when hovering or editing). We definitely need that distinction IMHO.
Furthermore, when comparing the single-color (Bojhan's) vs. dual-color (Kevin's, and in current patch) outline, I feel that the dual-color outline actually feels better, cleaner, crisper. So, I am *not* implementing this and I want Bojhan + Kevin to come to an agreement on what it should be. Right now only an incomplete solution has been proposed.


Round 4.

Patch attached for round 4. Uses git format-patch so you can assess the individual improvements.

New improvements in this round (on top of the improvements in round 1, 2 & 3, see #2, #16 & #26):

  • Use the hand cursor when hovering an editable field.
  • Fix toolbar pointer's background color.

Screenshots: see attachments, also in the issue summary.

Wim Leers’s picture

Issue summary: View changes

Round 3.

Wim Leers’s picture

Issue summary: View changes

Round 4.

tkoleary’s picture

@Bojhan

The ideal solution would be for us to provide two styles, one for dark themes and one for light themes and people can just easily implement one or the other (other theme variations, frankly just require a custom style). I'd imagine that is follow up, so for now I'd like to improve our light version and remove the second color.

This fails if the background color is the same color as the border

Wim Leers’s picture

#31: that doesn't really help move this forward. Bojhan made design changes in #18, you said you were okay with all of them except with the toolbar border's color. So you okay'd this specifically, because you were fine with everything else, this is part of everything else. I'm saying in #30 that I'd question whether this dual-color outline is good. I'm asking for the input from the two of you.

The input I need:

  1. Should we keep dual-color outlines?
  2. If not, then we move to single-color outlines, but what should their colors then be?
Bojhan’s picture

@tkoleary So, all fails if the color is the same as the background. There is really nothing you can do, unless you want a border thats adaptive to the background (goodluck to Wim :D with that).

The basic concern that I had was really the highlight color and selected color are virtually the same, its a blue and slightly darker blue - this difference can be hard to distinguish. I also noted that the "dual-color" outline made it significantly busier than needed. In many visual designs the 2nd color, if you are using a border with two colors is only slightly more or is used as shadow effect. In this case its actually used to emphasize, from my point of view is the highlight already pretty in your face - there is no need to emphasize it more.

tkoleary’s picture

@Bojhan

Not sure I was clear. The purpose of the two outlines is that there will never be a case where you don't see it (since the background can't be both colors).

Do you suggestions on what the two colors should be to keep them in line with the style guide?

webchick’s picture

Status: Needs review » Needs work

No longer applies. :(

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
33.54 KB

Rebased.

Wim Leers’s picture

The one thing we still had to resolve was the border around the field thing (see #30#34). It's extremely minor and something that can be solved later on, it shouldn't hold up all the other improvements in this patch, all of which are much more important.

Both Bojhan and Kevin agreed with this.

It still applies cleanly to HEAD. Bojhan or Kevin, please RTBC :)

tkoleary’s picture

RTBC!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

This is Kevin's first RTBC I think — @Kevin: you need to change the status field! :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

AWESOME. So excited to see this patch go in! :D

Committed and pushed to 8.x. Great work, all!

Wim Leers’s picture

Issue tags: -sprint

.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Overview of all changes.