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 box shadow, border, and notice for a template part editing indicators. #23406

Closed
wants to merge 7 commits into from

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Jun 23, 2020

Description

This explores adding a semi-transparent box-shadow to a template part in the editor when it is selected for editing.

This is an alternative to the suggested "Spotlight Mode" behavior for template part editing. Since that mode is already implemented on the individual block level, a box shadow may apply a similar style that is still distinctly different from and plays well with the spotlight mode.

While slightly rough and lacking on its own, this shadow may work well with other indicators that are being explored such as the renaming toolbar.

How has this been tested?

Screenshots

Before:

Screen Shot 2020-06-23 at 6 22 12 PM

After:

Screen Shot 2020-08-07 at 4 46 55 PM

1. Information about the change being persistent 2. Border visible when editing inner blocks 3. Overlay/scrim over rest of content when editing template part

2020-08-07 16 45 59

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@Addison-Stavlo Addison-Stavlo self-assigned this Jun 23, 2020
@github-actions
Copy link

github-actions bot commented Jun 23, 2020

Size Change: +753 B (0%)

Total Size: 1.16 MB

Filename Size Change
build/block-library/editor-rtl.css 8.45 kB +96 B (1%)
build/block-library/editor.css 8.45 kB +96 B (1%)
build/block-library/index.js 133 kB +361 B (0%)
build/edit-site/index.js 17 kB +41 B (0%)
build/edit-site/style-rtl.css 3.14 kB +80 B (2%)
build/edit-site/style.css 3.14 kB +79 B (2%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.97 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 125 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.6 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.5 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.4 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.38 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.23 kB 0 B
build/edit-navigation/index.js 10.9 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.62 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-widgets/index.js 9.38 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.33 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 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.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@Addison-Stavlo
Copy link
Contributor Author

Note - This idea would most likely require a function to determine the color/opacity of the shadow based on the current background color in the editor. 🤔 Otherwise this idea really won't work well for a variety of themes.

@noahtallen
Copy link
Member

I feel like this kind of thing might be "good enough" to start iterating on 🤔 Maybe also with a border or something

@Addison-Stavlo
Copy link
Contributor Author

I feel like this kind of thing might be "good enough" to start iterating on 🤔 Maybe also with a border or something

Agree on both. I pulled in changes from master now that it has width alignments, a renaming toolbar, and a border when the actualy template part block is selected (although we may want to have that border whenever inner blocks are selected as well?).

Template Part selected:
Screen Shot 2020-07-16 at 6 40 05 PM

Inner block selected:
Screen Shot 2020-07-16 at 6 40 14 PM

At first glance Im not exactly sure where that border is being applied, but it may look nice to have it applied with inner blocks selected as well.

@Addison-Stavlo Addison-Stavlo changed the title Try box shadow for a template part editing indicator. Try box shadow, border, and notice for a template part editing indicators. Aug 6, 2020
@noahtallen
Copy link
Member

Tried this out locally. Feels pretty nice, thanks for working on it!! I have some thoughts:

  • the blue focus border definitely feels a little bit off. I'm so used to only seeing it when I have the block selected that it feels weird when I am working with other blocks. Maybe the border needs to be separate from the "active selection" state?
  • There's a little issue with hover mode where it thinks the block is not hovered when going over whitespace in between child blocks:
    2020-08-06 14 54 25
  • I think after seeing this, I like the notice UI closer to the designs where it is attached to the template part itself. As is, I actually didn't see it at first. (And IMO, I think I'd prefer it being always visible rather than going away after some amount of time 🤔

Screen Shot 2020-08-06 at 2 54 47 PM

- I definitely like the "spotlight" around the template part when editing it. - Can we also apply these changes to the post-content block? IMO, it fits the same pattern really well.

@Addison-Stavlo
Copy link
Contributor Author

the blue focus border definitely feels a little bit off. I'm so used to only seeing it when I have the block selected that it feels weird when I am working with other blocks. Maybe the border needs to be separate from the "active selection" state?

By default the blue border is applied when the block is selected. The changes added here make it also applied when the children in the template part are selected, is this what feels off? i.e. the template part shouldn't have a blue border when we have selected one of its inner blocks? I feel like the box shadow looked really naked without the border, but it will be simpler to leave it 'as-is' and may feel less naked if we are also adding the spotlight mode to the box shadow.

There's a little issue with hover mode where it thinks the block is not hovered when going over whitespace in between child blocks:

Yeah, it seems like the block inserter there is picking up the hover state. I not sure how to get around this. 🤔

I think after seeing this, I like the notice UI closer to the designs where it is attached to the template part itself. As is, I actually didn't see it at first. (And IMO, I think I'd prefer it being always visible rather than going away after some amount of time

I think currently the notice options are to have the notice either at the top of the editor or on the snackbar. With the snackbar it seems to disappear after some time by default, I wonder if there is an option to disable this behavior.

I definitely like the "spotlight" around the template part when editing it. - Can we also apply these changes to the post-content block? IMO, it fits the same pattern really well.

Yeah, I think whatever visibility and notifications we settle on would make sense for post-content as well? It is the same in that it is a separate entity whose changes will be applied to everywhere it is used. Perhaps some other blocks as well? If it gets to a certain point maybe we should consider making some sort of hook or HOC to enable this functionality as opposed to writing it into every block. 🤔 although im not sure if there would be enough blocks needing this behavior to warrant that?

@noahtallen
Copy link
Member

Re the blue border, I think the idea is to have the blue block border be related to the block you are specifically editing. I think when selecting a child block, you are specifically editing that child block rather than the template part, so IMO the template part border should be slightly different when a child block is selected vs when itself is selected. (for example, i think the designs here have a gray border when a child block is focused)

Maybe even a lighter blue color to indicate that it's still related to what you're editing, but not the specific thing you're editing? 🤔

Yeah, it seems like the block inserter there is picking up the hover state

Oof, that's unfortunate :(

I wonder if there is an option to disable this behavior.

It almost feels like this isn't a normal notice but just some info about the block being edited 🤔 Maybe we should build it separately rather than using the notice lib?

I think whatever visibility and notifications we settle on would make sense for post-content as well

+1 for sure. In my mind, the distinction is an "inner block controller" should get this treatment. Maybe not something like the nav block or site title block. Post content and template part would both be examples of inner block controllers though

@Addison-Stavlo
Copy link
Contributor Author

(for example, i think the designs here have a gray border when a child block is focused)

Oh! I missed that (or misinterpreted it). Updated!

Yeah, it seems like the block inserter there is picking up the hover state

Oof, that's unfortunate :(

I wonder if we do something like add/remove an is-hovered class on mouseover / mouse leave if that would get around this? The __experimentalBlock wrapper seems to do this:

Although im not noticing it working for blocks that use that in the site editor? 🤔

It almost feels like this isn't a normal notice but just some info about the block being edited 🤔 Maybe we should build it separately rather than using the notice lib?

Yeah, I think that might make sense.

@noahtallen
Copy link
Member

I think that hover class is supposed to show up. I think some changes in this PR must remove it, because I noticed that it was working in #24394. 👀

@noahtallen noahtallen force-pushed the try/template-part-editing-visibility branch from 4002313 to 96754ad Compare August 6, 2020 23:57
@noahtallen
Copy link
Member

btw @Addison-Stavlo I rebased this on master and ended up having to squash all of the commits to avoid weird merge conflicts. my hunch is that we were missing a change related to the hover states, because they were showing up on master, but didn't show up in this branch. and no changes here should have caused them to go away :)

@noahtallen
Copy link
Member

The is_hovered class is showing up now, but it appears to be removed when you hover over a child block

@noahtallen
Copy link
Member

noticed a little bug. when first loading, the focus mode was still applied:

Screen Shot 2020-08-07 at 2 49 13 PM

just posting the screenshot here for discussion:

Template part block focused
Screen Shot 2020-08-07 at 2 50 50 PM

I'm kinda seeing four different areas of UI which feel as if they could be combined into the same area. in clockwise order:

  • block toolbar
  • template part name
  • information about changes applying in more than just this one place
  • the "header" tag (which is always visible even when selecting a child block

@noahtallen
Copy link
Member

#24450 moves the block title to the toolbar, which is a good way to help make things more consistent

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Aug 10, 2020

It almost feels like this isn't a normal notice but just some info about the block being edited 🤔 Maybe we should build it separately rather than using the notice lib?

I have been thinking about this more and am flip flopping a bit on my opinion. We probably should be using the notices package as much as we can. If the snackbar notice is not visible enough, we should add options to the notices package to allow for snackbar notices that do not dismiss automatically after X seconds or to increase their visibility in other ways. Notices are build with accessibility, screen reader, and other concerns in mind. It helps to utilize the package because as time goes on and notices are improved or iterated upon, our template part notice would receive these upgrades and remain consistent with the rest of Gutenberg. Building specific in-block notices seems like it could create a bit more pain down the line when they need to be updated and kept consistent, especially if we need to allow similar notices for other entity specific blocks.

Another thing to consider is that the snackbar is a tried and true place to show notices. Attaching them to the top or bottom of a block will become problematic as they won't be visible in various circumstances. If we scroll down to the footer and start editing the top blocks in it, the notice attached beneath it may remain off the screen and unseen. Similar issues with large template parts that may have larger height than the editor height (like maybe with post-content blocks later as they may need a similar notice). While the snackbar will always be visible in the editor.

Ex - editing the header on mobile:
Screen Shot 2020-08-10 at 12 23 22 PM

Or the footer in desktop browser:
Screen Shot 2020-08-10 at 12 23 41 PM

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Aug 10, 2020

noticed a little bug. when first loading, the focus mode was still applied:

Right, the setting persists beyond reload so if we have a Template Part selected when we close/reload the editor it remains active. So we could check for it and turn it off on editor load (in the same place you were experimenting with enabling select mode) 3a622df

Addison-Stavlo and others added 2 commits August 11, 2020 10:57
The initial implementation of template part notice reused the notice component from the component library and notice logic from core/notices.

After more discussion, we decided to try an alternative, custom implementation where the notice is attached to the template part block itself.
@jeyip jeyip force-pushed the try/template-part-editing-visibility branch from ce41a5f to 0cda5aa Compare August 11, 2020 18:08
@jeyip jeyip force-pushed the try/template-part-editing-visibility branch from 0cda5aa to 79e8ca4 Compare August 11, 2020 18:19
@aristath aristath deleted the try/template-part-editing-visibility branch November 10, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants