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

Blocks: Use a dropdown menu for the block's menu #2987

Merged
merged 3 commits into from
Oct 13, 2017

Conversation

youknowriad
Copy link
Contributor

This is the first step towards #2984 transforming the block's menu to a dropdown menu.

Future steps would be to:

  • Bring extra menu items (insert After, duplicate, switcher)
  • Implement and show the keyboard shortcuts.

screen shot 2017-10-11 at 13 51 07

@youknowriad youknowriad added [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Oct 11, 2017
@youknowriad youknowriad self-assigned this Oct 11, 2017
@youknowriad youknowriad requested a review from jasmussen October 11, 2017 12:51
@codecov
Copy link

codecov bot commented Oct 11, 2017

Codecov Report

Merging #2987 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2987      +/-   ##
==========================================
- Coverage   34.37%   34.29%   -0.09%     
==========================================
  Files         196      197       +1     
  Lines        5809     5913     +104     
  Branches     1027     1058      +31     
==========================================
+ Hits         1997     2028      +31     
- Misses       3222     3271      +49     
- Partials      590      614      +24
Impacted Files Coverage Δ
editor/block-settings-menu/content.js 0% <0%> (ø) ⬆️
editor/block-settings-menu/index.js 0% <0%> (ø) ⬆️
components/popover/index.js 86.86% <0%> (-1.05%) ⬇️
components/dropdown-menu/index.js 94.17% <0%> (-0.62%) ⬇️
editor/block-switcher/index.js 0% <0%> (ø) ⬆️
utils/focus/tabbable.js 100% <0%> (ø) ⬆️
components/navigable-menu/index.js 28% <0%> (ø)
blocks/library/freeform/old-editor.js 1.96% <0%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63cb942...15717d9. Read the comment docs.

@jasmussen
Copy link
Contributor

Pushed a little polish:

screen shot 2017-10-11 at 16 58 40

I'm going to take a look at hover and focus styles again. They are a little bit all over the place — my fault. But given we have some styles that work now from an a11n perspective, and fewer styles, I think I can take a look at that cohesively, separately.

It seems like the hover/focus style for the Settings item fires by just opening the menu, is that just me?

The keyboard shortcut hints is probably also best addressed separately.

One thing, if I open the popup on the demo content, then scroll, the popup doesn't stick to where it was opened. I sense this may be difficult to fix :(

@youknowriad
Copy link
Contributor Author

It seems like the hover/focus style for the Settings item fires by just opening the menu, is that just me?

That's the default accessible behavior added automatically to any dropdown

@youknowriad
Copy link
Contributor Author

youknowriad commented Oct 11, 2017

One thing, if I open the popup on the demo content, then scroll, the popup doesn't stick to where it was opened. I sense this may be difficult to fix :(

@aduth do you think #2966 could have an impact on this? Do you think we could have two popover providers, one inside the scrollable area and one outside it? or maybe provide a prop to use a relative position instead of a fixed position

@aduth
Copy link
Member

aduth commented Oct 11, 2017

Yikes, that's a tough one. Managing multiple popover slots sounds like a pain. The offset updating on window scroll should be positioning itself based on the viewport offset of the parent node to which it's attached. If this is not working correctly, it could be related to:

@youknowriad youknowriad force-pushed the update/block-settings-menu-dropdown branch from 0d6c7fa to 01198d4 Compare October 12, 2017 15:44
@youknowriad
Copy link
Contributor Author

Rebased this, the scroll issue should be fixed

@jasmussen
Copy link
Contributor

So nice. This works really well. I think it's 99% ready to merge.

There's a z-index issue that puts this above the editor bar:

screen shot 2017-10-13 at 09 52 20

— can you put this under it?

@jasmussen
Copy link
Contributor

Hoooraaay! 🚢

@youknowriad youknowriad merged commit cbab897 into master Oct 13, 2017
@aduth
Copy link
Member

aduth commented Oct 20, 2017

This didn't account for the mobile block toolbar, which now renders a second ellipsis for the menu dropdown in a block toolbar, which is also not reachable:

Mobile

@youknowriad youknowriad deleted the update/block-settings-menu-dropdown branch October 20, 2017 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants