Improve consistency of Shared and Permalink blocks UI. #6480

afercia · 2018-04-28T12:19:44Z

This PR tries to improve consistency of the Post Permalink and Shared Block UIs, specifically about buttons and interaction.

post permalink: changes "OK" to "Save"
shared block:
- removes primary style from the Save button
- removes the "Cancel" button
- closes the edit form when clicking outside of it, inspired by what react-click-outside does
- (for keyboard users, it already closes when pressing Escape)

Screenshot:

Fixes #6469

karmatosed

Looks good to me and thanks for the speed here.

chrisvanpatten · 2018-04-28T18:22:39Z

Should the “Permalink:” label also have the bolder font-weight?

aduth

This makes it impossible to edit the contents of the shared block, since editing is canceled when clicking outside the panel (into the block contents itself).

aduth · 2018-04-30T12:24:38Z

core-blocks/block/edit-panel/index.js

+	}
+
+	handleClickOutside( event ) {
+		if ( ! this.editForm.current ) {


When will this be the case?

The edit form is a separate component and mounted only when editing, If you "click outside" when not editing, then this ref DOM element is null.

But isn't the callback only bound to the click event when it is in-fact mounted?

Yes but when the SharedBlockEditPanel component mounts, there's no form:

In the scenario above, if you click outside you get TypeError: Cannot read property 'contains' of null.

The form is rendered only when isEditing.

aduth · 2018-04-30T12:25:34Z

core-blocks/block/edit-panel/index.js

+	}
+
+	componentDidMount() {
+		document.addEventListener( 'click', this.handleClickOutside, true );


I'd have considered suggesting withGlobalEvents, though it doesn't support useCapture. Out of curiosity, why was that needed here?

From the react-click-outside (which inspired this bit of code) readme: https://github.com/kentor/react-click-outside#details

The enhanceWithClickOutside function wraps the provided component in another component that registers a click handler on document for the event capturing phase. Using the event capturing phase prevents elements with a click handler that calls stopPropagation from cancelling the click event that would eventually trigger the component's handleClickOutside function.

karmatosed · 2018-04-30T12:47:08Z

@chrisvanpatten lets maybe check other labels and go with consistency. Either way lets use that as a pattern @afercia.

afercia · 2018-04-30T13:23:13Z

Rebased to solve merge conflict.

afercia · 2018-04-30T13:39:28Z

@aduth I've refactored that check, to make it more clear.

afercia · 2018-05-05T15:02:12Z

This makes it impossible to edit the contents of the shared block, since editing is canceled when clicking outside the panel

Ah, interesting. I've missed that the "Edit" button, which is visually associated to the shared block name also gives access to editing the contents of the shared block. A few considerations:

at this point, I'm not so sure that when clicking "Edit" focus should be moved to the name field, as that's not the only editable thing in the block and not necessarily the primary action
maybe the "Edit" button should be placed elsewhere, to make clear the whole block is editable
the "click outside" behavior doesn't make sense, I'd say to remove it and close after saving is successful
/Cc @aduth @karmatosed

aduth · 2018-05-10T12:41:28Z

the "click outside" behavior doesn't make sense, I'd say to remove it and close after saving is successful

Yep, this seems reasonable. The block was originally implemented with a click-outside behavior, and it was removed at some point (I believe as part of nested blocks implementation). Since it's a potentially destructive operation, either accidentally making global changes (if click outside saves) or accidentally losing unsaved changes by a careless click (if click outside cancels), I don't know that click outside is a desirable behavior to support.

aduth · 2018-05-10T12:42:46Z

editor/components/post-permalink/style.scss

@@ -29,6 +29,7 @@

 .editor-post-permalink__label {
 	margin: 0 10px 0 5px;
+	font-weight: 600;


Minor: Could / should we add  or  as a nested element of the label instead?

Not sure.  adds a semantic meaning of "strong emphasis" which we don't need here, this element is a label and doesn't need emphasis.  is only presentational, so at this point why not use just CSS?

 actually does have semantic meaning as a "bring attention to" element.

The  element represents a span of text to which attention is being drawn for utilitarian purposes without conveying any extra importance and with no implication of an alternate voice or mood, such as key words in a document abstract, product names in a review, actionable words in interactive text-driven software, or an article lede.

https://www.w3.org/TR/html53/textlevel-semantics.html#the-b-element
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/b

without conveying any extra importance .. etc.

The b element should be used as a last resort when no other element is more appropriate. In particular, headings should use the h1 to h6 elements, stress emphasis should use the em element, importance should be denoted with the strong element, and text marked or highlighted should use the mark element.

To me, this means it's basically a presentational element to "highlight" some text. So why we should use an additional element? For presentation, CSS is more appropriate. Also, the current official recommendation is HTML 5.2. Instead. 5.3 is still a working draft.

A main worry with styling was that we're not using a variable for the bold value, so if ever the typeface weight changes, there's more instances to update. Can probably be addressed by introducing a variable if it's really an issue.

Should be fine as-is.

karmatosed · 2018-05-10T19:55:28Z

Yep, this seems reasonable.

Just adding as pinged that I also agree.

afercia · 2018-05-14T16:41:32Z

I've removed the "click outside" behavior: the block name form now closes on Save.

As per moving focus when editing, I've tried to change it and still not fully convinced. I'd tend to think the root issue is in the placement of the "Edit" button.:

Since it's close to the name form, it's really not so clear that also the shared block content becomes editable. Focusing the name field, makes the previous content hard to get to for keyboard users, as they're forced to reverse-tab. Also, there's no visual indication that the block content is editable:

I've tried to move focus to the editable content: this ended up in duplicating most of the code already used in BlockListBlock > focusTabbable()
other options could be:
- place the "Edit" button in a different spot
- use two different "Edit" buttons, one for the block content, the other one for the block name

I'd appreciate some feedback, any thoughts welcome.

aduth · 2018-05-23T17:30:08Z

I can't speak to the design / UX impact of separate edit buttons, but so far as shifting focus to the editable content: it seems we lack but could benefit from a consistent programmatic way to focus a block's editable content. Both the SharedBlockEdit and BlockListBlock components render a BlockEdit component. Maybe we should move focusTabbable from BlockListBlock into BlockEdit, then both BlockListBlock and SharedBlockEdit could call the focusTabbable function from a ref assigned to their rendered BlockEdit.

Let me know if this sounds reasonable. I could assist with these changes if needed.

afercia · 2018-05-26T16:24:11Z

could call the focusTabbable function from a ref assigned to their rendered BlockEdit.

I've played a bit with this. Getting the ref is straightforward, but seems I'm not able to access the functions defined in the child (BlockEdit), maybe because it's wrapped in some HoCs? /Cc @aduth

aduth · 2018-05-29T14:51:01Z

Responding in passing, will give a closer look as I catch up. This may be where we need ref forwarding on our higher-order components (probably via the createHigherOrderComponent abstraction, cc @gziolo I think we may have mentioned this before?).

gziolo · 2018-05-29T18:44:39Z

Responding in passing, will give a closer look as I catch up. This may be where we need ref forwarding on our higher-order components (probably via the createHigherOrderComponent abstraction, cc @gziolo I think we may have mentioned this before?).

It might be very useful to have it built-in in createHigherOrderComponent. It looks like we have a valid use case to implement it. It seems also like it shouldn't be that difficult to implement given that the following example is very close to what we have:

function logProps(Component) {
  class LogProps extends React.Component {
    // ...
  }

  function forwardRef(props, ref) {
    return <LogProps {...props} forwardedRef={ref} />;
  }

  // Give this component a more helpful display name in DevTools.
  // e.g. "ForwardRef(logProps(MyComponent))"
  const name = Component.displayName || Component.name;
  forwardRef.displayName = `logProps(${name})`;

  return React.forwardRef(forwardRef);
}

There is one blocker, Enzyme version and it's lack of support for React 16.3 ...

nerrad · 2018-06-26T20:26:24Z

Regarding built-in ref forwarding in createHigherOrderComponent. I took an initial look at this intending to do a pull, but there's a couple things that might be blockers:

createHigherOrderComponent is not used by all GB HOCs. See withInstanceId and withFocusReturn as examples.
at least one HOC is already creating a ref - see withGlobalEvents.

It'd be a fairly breaking change to do directly on createHigherOrderComponent but probably okay to do it as a new method on wp.element. Maybe something like createHigherOrderComponentForwardingRef?

gziolo · 2018-06-26T20:40:06Z

Given that we usually compose HOCs it should be rather included in the existing helper method. Ideally, we should explore if we can detect if ref was passed and try to forward it or do nothing otherwise. Do you think it would be a breaking change? Do you have any specific use case where you know it would change behavior? So far it was rather a limitation.

nerrad · 2018-06-26T20:47:47Z

I shouldn't have been so imperative in my statement. Rather than "It'd be a fairly breaking change" I should have written "It might be a breaking change". I'll explore some more and ya I might be able to just detect if ref is already passed.

gziolo · 2018-06-26T20:58:50Z

Cool, we have time until 3.3 ships (2 weeks?) for breaking changes. In case we need them 😃

Another thing we still didn’t agree on is if we should move a few general HOCs and related helpers from element module to its own module. This was raised a few times because sometimes you need to pull in all components to just use HOC which is a more general concept, on the other hand composing HOCs isn’t really an essential part of element package 🤔

gziolo · 2018-08-08T11:20:27Z

@afercia can you update this code so we could review again?

afercia · 2018-08-08T12:49:59Z

Rebased keeping the change from #7987. Screenshot:

Note 1:
unrelated but the permalink input field misses the new border-radius style recently added /Cc @jasmussen

Note 2:
unrelated (happens also on master) the permalink UI needs some love in the responsive view, especially when the full URL is long. Screenshot from master:

Soean · 2018-10-12T20:00:05Z

I haven't seen this PR before... Most of the changed were merged in #7905 + #10563 missing from this PR. I opened a separate PR for the permalink label: #10566

afercia requested review from karmatosed and aduth April 28, 2018 12:20

karmatosed approved these changes Apr 28, 2018

View reviewed changes

gziolo added this to the 2.8 milestone Apr 30, 2018

aduth requested changes Apr 30, 2018

View reviewed changes

afercia force-pushed the update/6469-button-labels-styles-consistency branch from bff991d to e8de48e Compare April 30, 2018 13:22

aduth modified the milestones: 2.8, 2.9 May 3, 2018

aduth reviewed May 10, 2018

View reviewed changes

afercia force-pushed the update/6469-button-labels-styles-consistency branch from 36ffa4c to c97e34d Compare May 14, 2018 14:35

youknowriad modified the milestones: 2.9, 3.0 May 16, 2018

youknowriad modified the milestones: 3.0, 3.1 May 29, 2018

aduth mentioned this pull request May 30, 2018

Modal component #6261

Merged

4 tasks

aduth modified the milestones: 3.1, 3.2 Jun 21, 2018

nerrad mentioned this pull request Jun 27, 2018

WIP: Feature: Implement forwardRef in wp.element.createHigherOrderComponent #7557

Closed

10 tasks

aduth modified the milestones: 3.2, 3.3 Jul 6, 2018

aduth modified the milestones: 3.3, 3.4 Jul 18, 2018

pento modified the milestones: 3.4, 3.5 Jul 30, 2018

gziolo removed this from the 3.5 milestone Aug 8, 2018

gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Enhancement A suggestion for improvement. labels Aug 8, 2018

afercia added 4 commits August 8, 2018 14:12

Improve consistency of Shared and Permalink blocks UI.

08eeb76

Better check for the edit form ref, which exist only when isEditing.

4ae1867

Remove click outside behavior.

1c3eca3

Re-add changes from PR 7987.

9516744

afercia force-pushed the update/6469-button-labels-styles-consistency branch from a4aea06 to 9516744 Compare August 8, 2018 12:42

gziolo removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Aug 8, 2018

gziolo added this to the 3.6 milestone Aug 8, 2018

jasmussen mentioned this pull request Aug 15, 2018

Polish permalink UI and make it responsive #9010

Merged

youknowriad modified the milestones: 3.6, 3.7 Aug 15, 2018

youknowriad removed this from the 3.7 milestone Aug 30, 2018

Soean mentioned this pull request Oct 12, 2018

Bold permalink label #10566

Merged

Soean closed this Oct 12, 2018

afercia deleted the update/6469-button-labels-styles-consistency branch December 2, 2018 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve consistency of Shared and Permalink blocks UI. #6480

Improve consistency of Shared and Permalink blocks UI. #6480

afercia commented Apr 28, 2018

karmatosed left a comment

chrisvanpatten commented Apr 28, 2018

aduth left a comment

aduth Apr 30, 2018

afercia Apr 30, 2018 •

edited

Loading

aduth Apr 30, 2018

afercia Apr 30, 2018

aduth Apr 30, 2018

afercia Apr 30, 2018

karmatosed commented Apr 30, 2018

afercia commented Apr 30, 2018

afercia commented Apr 30, 2018

afercia commented May 5, 2018

aduth commented May 10, 2018

aduth May 10, 2018

afercia May 11, 2018

ZebulanStanphill May 12, 2018

afercia May 14, 2018

aduth May 14, 2018

karmatosed commented May 10, 2018

afercia commented May 14, 2018

aduth commented May 23, 2018

afercia commented May 26, 2018

aduth commented May 29, 2018

gziolo commented May 29, 2018 •

edited

Loading

nerrad commented Jun 26, 2018 •

edited

Loading

gziolo commented Jun 26, 2018 •

edited

Loading

nerrad commented Jun 26, 2018

gziolo commented Jun 26, 2018

gziolo commented Aug 8, 2018

afercia commented Aug 8, 2018

Soean commented Oct 12, 2018

Improve consistency of Shared and Permalink blocks UI. #6480

Improve consistency of Shared and Permalink blocks UI. #6480

Conversation

afercia commented Apr 28, 2018

karmatosed left a comment

Choose a reason for hiding this comment

chrisvanpatten commented Apr 28, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed commented Apr 30, 2018

afercia commented Apr 30, 2018

afercia commented Apr 30, 2018

afercia commented May 5, 2018

aduth commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed commented May 10, 2018

afercia commented May 14, 2018

aduth commented May 23, 2018

afercia commented May 26, 2018

aduth commented May 29, 2018

gziolo commented May 29, 2018 • edited Loading

nerrad commented Jun 26, 2018 • edited Loading

gziolo commented Jun 26, 2018 • edited Loading

nerrad commented Jun 26, 2018

gziolo commented Jun 26, 2018

gziolo commented Aug 8, 2018

afercia commented Aug 8, 2018

Soean commented Oct 12, 2018

afercia Apr 30, 2018 •

edited

Loading

gziolo commented May 29, 2018 •

edited

Loading

nerrad commented Jun 26, 2018 •

edited

Loading

gziolo commented Jun 26, 2018 •

edited

Loading