Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try: G2 polish #20578

Merged
merged 5 commits into from Mar 3, 2020
Merged

Try: G2 polish #20578

merged 5 commits into from Mar 3, 2020

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Mar 2, 2020

This PR checks a couple of the boxes on the list in #20575, these:

  • The direction that block toolbar menu items open in, seems arbitrary.
  • Renaming the "Publish..." button to just "Publish", as the ellipsis does not add value.

Partially this one:

  • Placeholders: consider simplifying further by merging the title with the description and removing the icon. This will scale better to very tiny states, necessary for patterns.

  • Notices behave inconsistently towards the mobile breakpoint.

I also explored this one:

  • Explore a directional "appear" animation to the block toolbar.

... but it did not feel right. It can potentially still work, but it's something to revisit.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Mar 2, 2020

Screenshots & GIFs:

dropdown

Screenshot 2020-03-02 at 13 27 55

Screenshot 2020-03-02 at 13 28 01

@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 2, 2020

Size Change: -332 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-editor/index.js 105 kB +131 B (0%)
build/block-editor/style-rtl.css 10.5 kB -18 B (0%)
build/block-editor/style.css 10.5 kB -17 B (0%)
build/block-library/editor-rtl.css 7.4 kB -263 B (3%)
build/block-library/editor.css 7.4 kB -264 B (3%)
build/block-library/index.js 116 kB +22 B (0%)
build/block-library/style-rtl.css 7.51 kB +17 B (0%)
build/block-library/style.css 7.51 kB +17 B (0%)
build/components/style-rtl.css 15.6 kB +36 B (0%)
build/components/style.css 15.5 kB +38 B (0%)
build/editor/index.js 44.6 kB -13 B (0%)
build/editor/style-rtl.css 3.98 kB -25 B (0%)
build/editor/style.css 3.98 kB -25 B (0%)
build/rich-text/index.js 14.3 kB +32 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.9 kB 0 B
build/edit-post/style-rtl.css 8.53 kB 0 B
build/edit-post/style.css 8.53 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Mar 2, 2020

Regarding G2, I love how you handled focus management in the toolbar. When tabbing it starts with movers even though they aren’t visible by default. ❤️

Did you consider having movers always visible when one of the toolbar items is focused? When the toolbar has only a few buttons, the experience isn't ideal:

g2-keyboard

Hover state works excellent in my opinion, I'm only concerned about the behavior when using arrow keys in the toolbar.

/**
* Renders a placeholder. Normally used by blocks to render their empty state.
*
* @param {Object} props The component props.
* @return {Object} The rendered placeholder.
*/
function Placeholder( {
icon,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 2, 2020

Contributor

That's something that should probably be communicated with a dev note. There's probably other code lines we can remove. (All the places where we pass an icon to the component)

This comment has been minimized.

Copy link
@jasmussen

jasmussen Mar 3, 2020

Author Contributor

If you like, we can do this as a separate PR, perhaps that's better? In fact it might even be good to postpone doing this, and doing it together with the separate task which is to combine the title and the description into one, i.e. deprecate the description.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Mar 3, 2020

Contributor

Ideally, the removal of the icon prop and its usage happens at the same time. Let's postpone this to its dedicated PR then.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Mar 3, 2020

Author Contributor

Done:

Screenshot 2020-03-03 at 09 30 10

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Mar 3, 2020

Did you consider having movers always visible when one of the toolbar items is focused? When the toolbar has only a few buttons, the experience isn't ideal:

Yep, and there are some switches and levers we can adjust to refine this specific interaction. For now we've gone with this and can revisit. Specifically for blocks like Social Links, Navigation and Buttons, I think there's more work to be done to make it trivial to select the block itself. The G2 focus improvements makes strides, but for example for social links, the URL pops out immediately which puts focus there. I think there's some refinement we can do there too.

jasmussen added 2 commits Mar 3, 2020
Copy link
Contributor

youknowriad left a comment

LGTM 👍

Looks like there's some unit tests snapshots to update

@jasmussen jasmussen merged commit 20186c9 into master Mar 3, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the try/g2-fixes branch Mar 3, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 3, 2020
@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Apr 1, 2020

The change to the notice action button (float: right added) doesn't seem ideal to me.

1
Large screen with a notice long text: position and alignments don't look so ideal:

Screenshot 2020-04-01 at 10 34 27

2
On a custom notice, it is possible to reset the default CSS classes and set custom CSS classes on the action button. See noDefaultClasses. By using className it is possible, for example, to add an is-link class to make the button look like a link. In this case, the layout doesn't look so ideal as well:

Screenshot 2020-04-01 at 10 34 30

3
By using __unstableHTML, it is possible to use HTML for the notice text. However, __unstableHTML renders an additional <div> element via the RawHTML component. This additional div breaks the flex layout:

Screenshot 2020-04-01 at 10 35 32

/Cc @jasmussen

4
Pre-existing issue: it seems to me there's something that doesn't work as expected in the RawHTML component. To my understanding, the wrapper div should be "stripped out by the serializer unless there are non-children props passed". However, seems to me it isn't stripped out.

/Cc @gziolo

/**
* Component used as equivalent of Fragment with unescaped HTML, in cases where
* it is desirable to render dangerous HTML without needing a wrapper element.
* To preserve additional props, a `div` wrapper _will_ be created if any props
* aside from `children` are passed.
*
* @param {Object} props
* @param {string} props.children HTML to render.
* @param {Object} props.props Any additonal props to be set on the containing div.
*
* @return {WPComponent} Dangerously-rendering component.
*/
export default function RawHTML( { children, ...props } ) {
// The DIV wrapper will be stripped by serializer, unless there are
// non-children props present.
return createElement( 'div', {
dangerouslySetInnerHTML: { __html: children },
...props,
} );
}

Will create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.