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

Focus Mode: Hide Block Outlines #9394

Merged
merged 16 commits into from
Aug 30, 2018
Merged

Focus Mode: Hide Block Outlines #9394

merged 16 commits into from
Aug 30, 2018

Conversation

youknowriad
Copy link
Contributor

Related #9334

This PR bootstraps the work on the focus mode proposed in #9334. Ideally, we'd be able to iterate and ship additions to the focus mode iteratively.

In this first step, I'm:

  • Renaming the option in the menu "Focus Mode" (note that the selectors, the state stays as is: isFixedToolbar, this can be changed later)
  • Removing the block outlines from the Focus Mode.
  • I'm also considering the focus mode available only in desktop (it was already the case for the fixed toolbar to top option)

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Aug 28, 2018
@youknowriad youknowriad self-assigned this Aug 28, 2018
@youknowriad youknowriad requested review from karmatosed, mtias, kjellr, jasmussen and a team August 28, 2018 09:34
return (
<MenuGroup
label={ __( 'Writing' ) }
filterName="editPost.MoreMenu.writing"
Copy link
Member

@gziolo gziolo Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this filter, it shouldn't be used in the long run. We should rather offer some fill if we want to make it possible to add an item to this group.

I can take care of it in the follow-up PR, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to ping you about it :)

@gziolo
Copy link
Member

gziolo commented Aug 28, 2018

This is looking really good so far. I will wait for the designers' feedback before going into a detailed review of all updated conditions inside BlockListBlock component 😄 I checked everything else and it looks good.

@jasmussen
Copy link
Contributor

Nice work! GIF:

gif

I don't think it was the intention for this mode to subsume the "Fix toolbar to top" option. I think Matías noted he'd like that to still be a separate option.

Should we hide the hover labels in this mode? Perhaps even the movers?

@tofumatt
Copy link
Member

As much as I loathe adding extra options, I think focus mode and fixing the toolbar make sense as separate options. Or rather, I think you should be able to fix the toolbar without enabling focus mode. With focus mode on you want the toolbar/editor chrome as out-of-the-way as possible, so enabling both when focus mode is on makes sense. But wanting the toolbar fixed doesn't mean you want focus mode on, as it's more aggressive in the editor chrome it (should/will) hide.

The states available should be:

  • No focus, no fixed toolbar
  • No focus, yes fixed toolbar
  • Yes focus, yes fixed toolbar (enabling focus enables them both)

I guess the Fixed Toolbar should be an independent option that we respect when Focus is off. With Focus on, even if the toolbar is set to not be fixed, we fix it for the duration of focus mode.

I'm a ramble-machine this morning, apparently.


Hiding the hover and movers in focus mode is 👍 from me.

@youknowriad
Copy link
Contributor Author

I don't think it was the intention for this mode to subsume the "Fix toolbar to top" option. I think Matías noted he'd like that to still be a separate option.

Happy to implement whatever works best. From the discussions on the issue, I understantood that we should gather all these changes in the "focus mode", I might have misunderstood it?

@kjellr
Copy link
Contributor

kjellr commented Aug 28, 2018

This is looking great so far! 👏

Happy to implement whatever works best. From the discussions on the issue, I understantood that we should gather all these changes in the "focus mode", I might have misunderstood it?

My understanding (but also my personal opinion) was that we'd gather these all into one mode. I outlined thinking around that here, but it boils down to keeping things simple and also helping us clearly define each editing mode. Happy to reconsider.

Should we hide the hover labels in this mode? Perhaps even the movers?

In my initial mockups, I included the labels, but showed them on a delay (as per #9197) and toned them down visually (text set in $dark-gray-700 on $light-gray-100):

That said, I could definitely see an argument to hiding them altogether here. Movers would also make sense to hide. Let's give it a try. We can always tone it back if we go too far. 🙂

@youknowriad
Copy link
Contributor Author

Agreed we should hide the labels, I'll do that.

Movers and block settings menu: We already hide them unless you're close enough. It's not perfect but for the sake of iterating, I prefer to keep them for now.

@ZebulanStanphill
Copy link
Member

Personally, I would prefer if Fix Toolbar to Top was a separate option, but I do not feel that strongly about it, and getting some iteration of Focus Mode (or whatever it should be called) in is better than no iteration.

Movers and block settings menu: We already hide them unless you're close enough. It's not perfect but for the sake of iterating, I prefer to keep them for now.

Notably, the ellipsis may be moving to the toolbar soon. The movers are still staying on the side, though. See #9074 and #9282.

@@ -129,12 +132,13 @@ const applyWithSelect = withSelect( ( select ) => {
const { getEditedPostAttribute, getEditorSettings } = select( 'core/editor' );
const { getPostType } = select( 'core' );
const postType = getPostType( getEditedPostAttribute( 'type' ) );
const { titlePlaceholder } = getEditorSettings();
const { titlePlaceholder, hasFixedToolbar } = getEditorSettings();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If removing the fixed toolbar option, should we rename hasFixedToolbar property to isFocusModeEnable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afercia mentioned that the mode should probably not be called "Focus Mode" because it could cause confusion with accessibility tools that have modes with the same name, so I would hold off on renaming variables until we know what the mode should be called.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. As others referred, I would also prefer to have the Fixed toolbar as an independent option that is forced by the focus mode.
Another feature for focus mode may be hiding the sidebars, so we get clean interface. This is not a requirement of this PR and may be iterated/tried in the future.

@youknowriad
Copy link
Contributor Author

I pushed some updates to the PR:

  • I separated the two options (personally, I don't feel it's a great result :) )
  • I added the "focus" style similar to Try: "Focus Mode". #8920

Thoughts?

@jasmussen
Copy link
Contributor

This is looking pretty close now:

gif

One thing stands out:

now

The least we can do sans block outlines is to align to the text in focus mode:

minimal-change

But we could also go slightly crazy and do this:

popout

That is, we center the toolbar and add a little triangle connector. Note that the separators between the segments would have to be smaller too, so as to allow the white padding of the "material" to extend into the triangle.

@jasmussen
Copy link
Contributor

Oh one note — with the dimming in place, if you multi select blocks, all blocks are dimmed. We might want to disable dimming for multi select.

@mtias mtias added this to the 3.7 milestone Aug 29, 2018
@folletto
Copy link
Contributor

The least we can do sans block outlines is to align to the text in focus mode

This feels coherent to me :)

@kjellr
Copy link
Contributor

kjellr commented Aug 29, 2018

This is looking great! @jasmussen's toolbar suggestions are fantastic: The left-aligned version feels right to me.

A couple quick questions:

  • When you switch to the Code Editor, the "Fix Toolbar to Top" and "Focus Mode" options are still visible, though they have no effect on your experience. Should these be hidden until you switch back to the Visual Editor?
  • Can we try adding a super-quick (60-100ms) fade to the blocks in this mode? When big images fade in and out, the instant opacity change can be a little jarring.

@youknowriad
Copy link
Contributor Author

Tweaked the toolbar and the animation.

@kjellr
Copy link
Contributor

kjellr commented Aug 29, 2018

The animation looks great for focused/unfocused items! Much more calming.

In Safari, I do see some weird jigglyness on hover though:

hover

This doesn't happen in Chrome/Firefox.

@mtias
Copy link
Member

mtias commented Aug 29, 2018

Looking over the different open issues and PRs for these modes. Let's go with the separate options as it seems to be the least prescriptive about what you should use. Any mode can be used for writing, it's a matter of preference and creative context.

We can revise later, but let's look at the following split and the following names:

  • No Option: This is the default. Can incorporate some of @kjellr’s ideas for delayed outlines / hovers.
  • Option 1: Unified Toolbar: Moves toolbar to the top — as it does now — but can also add some more distraction-free improvements by removing outlines altogether and showing the label in gray on delayed hover, etc.
  • Option 2: Spotlight Mode: Removes outlines, dims-down other blocks slightly, hides header elements and block movers. Toolbar renders like @joen’s proposal above Focus Mode: Hide Block Outlines #9394 (comment) by the block. This corresponds to the current PR.
  • Option 3: Full Screen. Self explanatory. Let's consider this separately.

Possibilities:

  • Option 1 + Option 2: Combines focus mode with header toolbar.
  • Any mode could be made full-screen, including "Code Editor".

Let's also group these settings as "Writing" in the ellipsis menu initially, and let's also move "Code Editor" as a single option (no visual editor) to the bottom group. Once in "Code Editor" the irrelevant options are inactive (focus / toolbar), or entirely hidden in favor of potential "Code Editor" specific features (like an eventual enabling syntax highlighting, for example).

@kjellr
Copy link
Contributor

kjellr commented Aug 29, 2018

Let's go with the separate options as it seems to be the least prescriptive about what you should use.

Let's also group these settings as "Writing" in the ellipsis menu initially, and let's also move "Code Editor" as a single option (no visual editor) to the bottom group. Once in "Code Editor" the irrelevant options are inactive (focus / toolbar), or entirely hidden in favor of potential "Code Editor" specific features (like an eventual enabling syntax highlighting, for example).

Here's a representation of the ellipsis menu with these changes:

screen shot 2018-08-29 at 2 05 41 pm

(or, with disabled options when the Code Editor is active)

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Aug 29, 2018

Instead of "Writing" that header could be something like "Preferences" or "Interface" or something, maybe? The options are not necessarily writing specific.

Also like the disabled options over the hidden options for Code Editor; and I really like that it shifts Code Editor to Tools. The classic editor puts Code at parity with Visual but I think with the way Gutenberg treats HTML, de-emphasising it—albeit only slightly—is probably a safer move.

(Not required in this PR but it would be nice for those UI preferences to be persisted to the server, rather than only in local storage, for moving between browsers/devices.)

@mtias
Copy link
Member

mtias commented Aug 29, 2018

(Not required in this PR but it would be nice for those UI preferences to be persisted to the server, rather than only in local storage, for moving between browsers/devices.)

I think this depends on whether preferences might be considered context or behaviour specific — I can configure my mobile version a certain way, and the desktop one differently.

@@ -101,6 +101,16 @@
.editor-block-list__block-edit .reusable-block-edit-panel * {
z-index: z-index(".editor-block-list__block-edit .reusable-block-edit-panel *");
}

&.is-focus-mode:not(.is-multi-selected) {
opacity: 0.6;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try 0.5 here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also apply to the title.

Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the refinements. This is looking solid for me as an iteration. I'm loving the possibilities by combining Spotlight and Unified Toolbar. In the future we should find some nice keyboard shortcuts so that you can effortlessly jump in and out as needed.

@@ -42,7 +42,7 @@ describe( 'block deletion -', () => {

describe( 'deleting the third block using the Remove Block shortcut', () => {
it( 'results in two remaining blocks and positions the caret at the end of the second block', async () => {
await pressWithModifier( [ 'Shift', META_KEY ], 'x' );
await pressWithModifier( [ 'Shift', META_KEY ], 'Backspace' );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the e2e tests because this was not updated. Not sure when this shortcut changed but was left as is in the e2e tests

cc @talldan @jasmussen

This is the right shortcut to delete the block, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Yeah would be nice to have @talldan answer here. I know that the backspace keyboard combo needs to be changed, but a different change was also merged in and later reverted. Perhaps this PR was created before the latter change was reverted? I'm pretty sure it's Backspace in master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I checked the code and it's backspace, and I saw the revert commit but for some reason the revert didn't revert the e2e test and it wasn't failing in e2e tests. Maybe the e2e test was added in parallel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad Sorry about that, the test was added in parallel, but I missed reverting it back before flying out to our meetup. Here's a PR that should fix things:
#9477

@jasmussen
Copy link
Contributor

Focus mode is looking really good. Nice work all around.

I don't know if we can still get a quick fix in for this, but the title fades out when you're in isEditing mode:

title

Also not at all an issue, not blocking at all, but wanted to note:

screen shot 2018-08-30 at 17 12 16

Multi select is cool, but now that the toolbar is aligned to the content instead of the boundary it sits a little funny there. Separately, in the future, we could look at making it so multi select only selects the content in focus mode, rather than have those extra padding. Or we could revisit https://user-images.githubusercontent.com/1204802/44781443-b70e4a80-ab84-11e8-8d56-0bbf9c29d789.png ;)

@youknowriad
Copy link
Contributor Author

Good catch @jasmussen I've pushed fixes for both issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.