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.

Sign up for GitHub

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

Jump to bottom

Add toolbar dropdown component for use with blocks #3904

Merged
merged 4 commits into from
Jan 21, 2021
Merged

Add toolbar dropdown component for use with blocks #3904

merged 4 commits into from
Jan 21, 2021

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Jan 18, 2021 •
edited
Loading

Changes proposed in this Pull Request

  • This PR introduces a generic dropdown to be used in toolbars, so we can use that for the restriction block, for the quiz blocks, and so on. Part of the code was borrowed from Add Restricted Course Content block #3849. Thank you, @gkaragia!

Testing instructions

You can test that in the add/lesson-actions-preview WIP branch.

  • Go to the lessons editor, and add the Lesson actions block.

Screenshot / Video

Screen.Recording.2021-01-18.at.17.31.42.mov

@renatho renatho added this to the 3.8.0 milestone Jan 18, 2021
@renatho renatho requested a review from a team January 18, 2021 20:33
@renatho renatho self-assigned this Jan 18, 2021
@alexsanford
Copy link
Contributor

Does this differ significantly from the DropdownMenu component which is used in core?

@yscik
Copy link
Contributor

yscik commented Jan 18, 2021 •
edited
Loading

Not well-documented but <ToolbarGroup> also has an isCollapsed option which turns it into a dropdown, wrapping a DropdownMenu with similar functionality. Example in core
(Not 100% sure that these are there in all our supported Wordpress versions)

@gikaragia
Copy link
Contributor

gikaragia commented Jan 19, 2021 •
edited
Loading

(Not 100% sure that these are there in all our supported Wordpress versions)

Did a bit of investigation on how this page is generated and after ending up on this comment it seems that the actual Gutenberg version that is included on each WP version is the latest one. Not sure what is the reason for including earlier versions in a range.

Previously I thought that for Wordpress 5.4 we needed to support Gutenberg v6.6.0 (which does not include <ToolbarGroup>) but we actually need to support v7.5.0-rc.1 (which it does) so it is safe to use.

On another note, a text argument was also added in <DropdownMenu> in Gutenberg v9.3 (not yet released).

@yscik
Copy link
Contributor

yscik commented Jan 19, 2021

Nice! For a text button in the toolbar, I used ToolbarGroup's toggleProps in this Syntaxhighlighter PR.

@renatho
Copy link
Contributor Author

renatho commented Jan 19, 2021

Thank you for checking and for the suggestions!

I tried to implement it using the ToolbarGroup component. But I faced 2 issues:

  • The toggleProps.children is not supported in the 7.5.0-rc.1 Gutenberg version. And I didn't find any possible workaround.
  • It doesn't support the "Preview lesson state" label designed. A workaround would be adding the title in the ToolbarGroup children.

The same happens with the DropdownMenu, for the same reasons.

I just replaced the Button in the list with MenuItem component, since it seems more appropriate for this case.

gikaragia
gikaragia reviewed Jan 20, 2021
View reviewed changes
Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

I am going to start using this in the restricted content block. Left a minor comment that came up while using it.

assets/blocks/editor-components/toolbar-dropdown/index.js Outdated
* @param {string} [props.optionsLabel] Options label.
* @param {Object} props.icon Icon for the toolbar.
* @param {string} props.value Current value.
* @param {Function} props.onChange Change function.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the arguments are a bit generic and the documentation don't specify what they actually do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this change is enough? 6a94e1e

gikaragia
gikaragia reviewed Jan 20, 2021
View reviewed changes
assets/blocks/editor-components/toolbar-dropdown/index.js
const selectedOption = options.find( ( option ) => value === option.value );

return (
<Dropdown
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the border to have a block color as the rest of the dropdown menus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed here: bbdf53c

@gikaragia gikaragia mentioned this pull request Jan 20, 2021
Merged
@renatho renatho mentioned this pull request Jan 20, 2021
Merged
gikaragia
gikaragia approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

LGTM!

@renatho renatho merged commit dfadb70 into feature/lessons-block Jan 21, 2021
@renatho renatho deleted the add/toolbar-dropdown branch January 21, 2021 13:37
@donnapep donnapep changed the title Add toolbar dropdown component for the editor Add toolbar dropdown component for use with blocks Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gikaragia gikaragia gikaragia approved these changes

Assignees

@renatho renatho

Labels
[Type] Enhancement
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@renatho @alexsanford @yscik @gikaragia