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: Put anchor inside block inspector's Advanced section #3475

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 14, 2017

Description

Related to: #3318, #3472.

As noted in here, with #3318 we introduced new ways to make blocks extensible. To make it happen we moved anchor out of the Advanced section in the block's toolbar. This PR tries to bring back this section in a way which allows to work it with the extensibility layer. To make sure that Advanced section gets displayed only when it contains items to render I used render prop technique. This is the same pattern @youknowriad used when refactoring and extracting reusable <Dropdown /> component.

How Has This Been Tested?

  1. Open Gutenberg editor.
  2. Add a Paragraph block.
  3. Open block settings.
  4. Make sure there are no advanced settings.
  5. Add a Heading block.
  6. Open block settings.
  7. Make sure there are advanced settings in the collapsed state.
  8. Make sure they open and work properly after you click toggle icon.
  9. Add an Image block.
  10. Make sure there are advanced settings in the collapsed state.
  11. Make sure they open and work properly after you click toggle icon.

Screenshots (jpeg or gifs if applicable):

Before

screen shot 2017-11-14 at 15 34 29

After

screen shot 2017-11-14 at 15 27 29

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

TODO:

  • Update documentation.

@codecov
Copy link

codecov bot commented Nov 14, 2017

Codecov Report

Merging #3475 into master will increase coverage by <.01%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3475      +/-   ##
==========================================
+ Coverage   34.24%   34.25%   +<.01%     
==========================================
  Files         257      257              
  Lines        6739     6744       +5     
  Branches     1222     1223       +1     
==========================================
+ Hits         2308     2310       +2     
- Misses       3740     3743       +3     
  Partials      691      691
Impacted Files Coverage Δ
blocks/hooks/anchor.js 80% <ø> (ø) ⬆️
...or/components/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
components/slot-fill/slot.js 66.66% <100%> (ø) ⬆️
blocks/inspector-controls/index.js 81.81% <50%> (-7.08%) ⬇️
components/slot-fill/fill.js 72.72% <0%> (-2.28%) ⬇️

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 0aee803...4af0f3b. Read the comment docs.

@gziolo gziolo self-assigned this Nov 14, 2017
@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Feature] Extensibility The ability to extend blocks or the editing experience labels Nov 14, 2017
@gziolo gziolo requested review from youknowriad and aduth November 14, 2017 14:41
@gziolo
Copy link
Member Author

gziolo commented Nov 14, 2017

I will add documentation when we agree that we want to add this functionality back using the proposed technique.

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.

I tested and this works as expected. The code looks fine to me! The solution to extend advanced controls and hide the panel if no advanced controls exist is really nice.

@youknowriad
Copy link
Contributor

youknowriad commented Nov 15, 2017

While this is good code, and a simple fix. I'm not sure we want to add an new "Slot" just for this. It seems to me that we can leverage the existing API and compose the two anchor/classname extensions to add a single panel instead of having two separate extensions.

I guess the question is whether we want to make this "advanced controls" slot an option for plugin authors or not. My personal opinion is no, because it's just a special case of the inspector controls slot.

Might be good to have another opinion cc @aduth

@gziolo
Copy link
Member Author

gziolo commented Nov 15, 2017

I guess the question is whether we want to make this "advanced controls" slot an option for plugin authors or not. My personal opinion is no, because it's just a special case of the inspector controls slot.

If we don't want to open advanced controls for plugin developers then we should definitely compose those two existing extensions. It should be very straightforward to implement with the existing extensibility capabilities 👍

Let's see what Andrew thinks before I remove this code :)

@aduth
Copy link
Member

aduth commented Nov 15, 2017

It could tie into the discussion of #1352. The whole "Advanced" grouping seems like an ad hoc placement for miscellaneous fields, those which might be common to many block types but not frequently used. This seems as if it would be more common to these core features than they would be leveraged by plugins, so I think it'd be safe to start conservatively without a standalone slot and compose the two features we have using it.

</PanelBody>
<Slot
name="Inspector.AdvancedControls"
renderFills={ ( fills ) => {
Copy link
Member

@aduth aduth Nov 15, 2017

Choose a reason for hiding this comment

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

I think this is a valuable feature (to hook into rendering when we know that fills exist, or alternatively when no fills exist, see #1343).

Do you think it might be more simple to use if implemented as a Function-as-Child (a more common pattern)?

<Slot name="Inspector.AdvancedControls">
	{ ( fills ) => <div>{ fills }</div> }
</Slot>

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is another way of doing it. Personally, I feel a bit awkward about treating this.props.children as a function. However, I'm fine with both patterns as long as they do their work :)

@gziolo
Copy link
Member Author

gziolo commented Nov 15, 2017

Closing this one based on comments from @youknowriad and @aduth. Thanks for your feedback together with @jorgefilipecosta.

This PR still looks useful if we ever need to add some conditional display for the content that depends on the fills.

@gziolo gziolo closed this Nov 15, 2017
@gziolo gziolo deleted the update/anchor-advanced-controls branch November 15, 2017 19:04
@gziolo gziolo restored the update/anchor-advanced-controls branch April 3, 2018 09:39
@gziolo gziolo deleted the update/anchor-advanced-controls branch April 3, 2018 09:45
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 [Feature] Extensibility The ability to extend blocks or the editing experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants