Skip to content

Conversation

@dkotter
Copy link
Collaborator

@dkotter dkotter commented Apr 8, 2021

Description of the Change

The default block editor mode is fullscreen, which hides the top admin bar. This admin bar is where Distributor puts the distribution tools. So when in fullscreen mode, it can be hard to figure out how to actually distribute something.

This PR attempts to solve that by adding a new Distributor panel to the editor with a toggle option. This panel will hold the current Distributed to and Distributed on messages that previously showed in the Post Status panel and will now show a new button to toggle the showing of the admin bar (if needed) and the showing of the Distributor menu content.

This option will show both in normal mode and in fullscreen mode on a published item that isn't a previously Distributed item (an item received from another site). On a non-published item, we output a simple message letting users know they need to publish first. On a newly published item, since the admin bar only updates on page refreshes, instead of showing the toggle we show a message letting users know they need to refresh.

Note: no copy or designs are final. Open to suggestions on all of that.

Screenshots of all scenarios will follow.

Screenshots

Post received through Distribution, shows the same in both normal and fullscreen mode:
distributed-post-full-and-normal

Post that has been Distributed, in normal and fullscreen mode
Screen Shot 2021-04-09 at 10 42 40 AM

Post with no Distributions, in normal and fullscreen mode
Screen Shot 2021-04-09 at 10 52 14 AM

Non-published post
non-published-post

Newly published post, prior to a page refresh
Screen Shot 2021-04-09 at 10 45 56 AM

Alternate Designs

Could attempt to move the full Distribution capabilities from the admin bar to this new panel.

Benefits

When using the block editor in fullscreen mode, it should now be easier to figure out how to distribute something

Possible Drawbacks

None

Verification Process

  1. Checkout this PR
  2. Test the various scenarios:
    • Edit a published item that hasn't been distributed yet, checking both normal mode and fullscreen mode
    • Edit a published item that has been distributed, checking both normal mode and fullscreen mode
    • Edit an item that hasn't been published yet, checking both normal mode and fullscreen mode
    • Edit an item that was received through distribution, checking both normal mode and fullscreen mode
  3. Ensure the new Distributor panel shows properly in all scenarios and toggling to the admin bar works properly in fullscreen mode

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#597

…anel. Add option to show/hide the admin bar, to more easily toggle that when in fullscreen mode. Also add helper text when not in fullscreen mode to let people know to look at the admin bar
@dkotter dkotter self-assigned this Apr 8, 2021
Darin Kotter added 4 commits April 9, 2021 09:37
…s, so they can be imported as react components. Add some styling to ensure icon isn't too large
…) but showing the Distributor menu. This works in both fullscreen and normal mode. For newly published items, since the admin bar won't exist until a refresh, add a message about this instead of showing toggle button
@jeffpaul jeffpaul added this to the 1.7.0 milestone Apr 9, 2021
@jeffpaul jeffpaul requested a review from dinhtungdu April 9, 2021 20:55
dinhtungdu
dinhtungdu previously approved these changes Apr 13, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@jeffpaul
Copy link
Member

@helen any further UX tweaks you'd like to see here before this gets merged in?

@jeffpaul jeffpaul requested a review from helen April 21, 2021 01:59
@jeffpaul jeffpaul modified the milestones: 1.7.0, 1.6.5 Apr 27, 2021
@jeffpaul jeffpaul mentioned this pull request Jun 7, 2021
16 tasks
@jeffpaul
Copy link
Member

Noting this might also handle #141, so will want to test for that in reviewing this PR.

@helen
Copy link
Contributor

helen commented Jul 22, 2021

This is working for me and I think does at least address the immediate problem - the one thing I think it needs before merge is to close the popup as soon as you click outside of the area, otherwise it's not super intuitive how you can close it (either click the button again or else mouse into the area and back out) and you can end up clicking around fruitlessly wondering if it'll ever close, and it does let you click into blocks, etc.

Darin Kotter added 2 commits July 23, 2021 14:27
… Listen for clicks outside the Distributor dropdown menu and close the menu when that happens
@dkotter
Copy link
Collaborator Author

dkotter commented Jul 23, 2021

@helen @dinhtungdu

close the popup as soon as you click outside of the area

Just pushed a fix for this

@dinhtungdu
Copy link
Contributor

@darin, @helen Clicking outside the area not only closes the popup but also selects the element under the mouse. For example when the popup is open after clicking on Distribut Post, click on the editor area selects the block then change the sidebar content, which may be confusing. The situation goes worse if Move to Trash button is under the cursor.

…ead of just adding styles to the open menu item, add an actual overlay div. We can then use this to capture clicks, which makes it easier to know when to close the menu when a click happens outside of it and ensures clicks don't impact any elements beneath
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 5, 2021

@dinhtungdu I've made some more changes here on how we handle the overlay. Previously this was just added as styles to the menu item. I've now added an actual overlay div that we show/hide when needed. This is then used to capture clicks so you don't end up clicking on any elements that show beneath that overlay.

This is a bigger change here though so would appreciate some testing. It seemed to work fine in my tests, both in the admin and in the front-end, and both with hovering over the menu, entering the menu via the keyboard and using the new sidebar button.

dinhtungdu
dinhtungdu previously approved these changes Aug 6, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu 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 working great in my test. But there are some minor issues with this PR:

  • We don't show the push menu on mobile/small screens, so please considering hiding the sidebar button on small screens.
  • It throws three errors in the console. It only happens with feature/gutenberg-fullscreen-support. I don't get any error with develop.

Screen Shot 2021-08-06 at 11 16 41

import { wp, dtGutenberg } from 'window';
import PluginIcon from '../img/icon.svg'; // eslint-disable-line no-unused-vars

const { Icon } = wp.components; // eslint-disable-line no-unused-vars
Copy link
Contributor

@dinhtungdu dinhtungdu Aug 6, 2021

Choose a reason for hiding this comment

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

Look like we haven't added Eslint rules for react yet. I think it's better to do it in this PR so we don't have to add these comments to disable Eslint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dinhtungdu Any recommendations on the best way to add these rules without impacting other things? We currently use an older version of the @10up/eslint-config and I updated that to the latest version (following instructions here), but I find that brings over a lot of pretty strict rules, some of which I don't agree with (which is fine, that's just my opinion).

But what this does mean is all the JS files will need a bunch of minor formatting changes applied in order to pass these new rules. Not sure we want all of that cleanup work on this PR, so wondering if there's either a simpler solution for now to add better React eslint rules or if we tackle this in a separate PR, one built around updating our entire build/lint process?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with (1) resolving those eslint failures in a separate PR and (2) not worrying about those until the next release (aka let's not block this issue/release by those eslint issues).

Copy link
Contributor

Choose a reason for hiding this comment

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

But what this does mean is all the JS files will need a bunch of minor formatting changes applied in order to pass these new rules.

I didn't think about this. Let's fix the eslint in other PR.

@dinhtungdu dinhtungdu dismissed their stale review August 6, 2021 07:38

I clicked on the wrong button

@dkotter
Copy link
Collaborator Author

dkotter commented Aug 19, 2021

@dinhtungdu

We don't show the push menu on mobile/small screens, so please considering hiding the sidebar button on small screens.

I've added some CSS to hide the Distributor panel on smaller screens

It throws three errors in the console

I actually don't see any errors. Any other information you can provide around where the errors are happening?

@jeffpaul jeffpaul requested a review from dinhtungdu August 20, 2021 16:06
@helen
Copy link
Contributor

helen commented Aug 24, 2021

There are two things going on here that I would like to see ironed out:

  • The admin toolbar remains visible after the dropdown closes, i.e. the is-showing-distributor class is not being removed
  • The overlay shows up before the dropdown because of the hoverIntent stuff on the toolbar, which feels really weird and can also lead to the overlay showing up and then never being removed again if you mouse over the Distributor item without triggering the dropdown.

@dinhtungdu
Copy link
Contributor

I actually don't see any errors. Any other information you can provide around where the errors are happening?

I tested on a multisite installation. Only Distributor is activated (network-wide). The WP version is 5.8. I'm using apache2 with PHP 7.3.24. Please check this screencast: https://share.getcloudapp.com/Jruxg2pn!

…over to try and deal with the hoverintent delay. Add better support for when distribution is in progress and you move out of the dropdown. Hide the admin bar when everything else closes in full screen mode
@dkotter
Copy link
Collaborator Author

dkotter commented Aug 27, 2021

@helen

The admin toolbar remains visible after the dropdown closes, i.e. the is-showing-distributor class is not being removed

This should be fixed now

The overlay shows up before the dropdown because of the hoverIntent stuff on the toolbar

I took another stab at this and I think it's better now, though still not perfect. As you mention, core uses hoverintent on the admin bar to determine when to show things. That has a timeout on it so you have to actually hover over something for a certain period of time before it triggers. This can result in us showing the overlay even though the dropdown isn't showing or cause a delay between the two.

I've added a delay on our code (if things aren't already showing) to try and deal with this. I feel it's definitely better but open to other ideas on how to solve this.

@dkotter
Copy link
Collaborator Author

dkotter commented Aug 27, 2021

@dinhtungdu Thanks for the additional details. I have a very similar set up but still don't get those warnings. But looking at the stack trace it gives, I think I found the issue and I've pushed a fix for that. Let me know if that clears those up for you or not.

Copy link
Contributor

@helen helen left a comment

Choose a reason for hiding this comment

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

I hope we'll be able to come up with a better long-term solution someday where we can remove a lot of this because it's pretty funky, but it's working for what we need for the moment.

@helen helen merged commit e8d963e into develop Aug 31, 2021
@helen helen deleted the feature/gutenberg-fullscreen-support branch August 31, 2021 17:00
@dkotter dkotter mentioned this pull request Sep 1, 2021
6 tasks
@dkotter
Copy link
Collaborator Author

dkotter commented Sep 1, 2021

@helen I agree and after doing some more thinking, I did think of a few ways to simplify this logic. That is in a new PR, #790

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.

Distributor admin bar menu unavailable in Gutenberg fullscreen mode (current WP default)

4 participants