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

fix/54877: Allow insertion of Buttons in the Navigation Block #55126

Closed
wants to merge 6 commits into from

Conversation

Sidsector9
Copy link
Contributor

What?

In this PR, the directInsert prop is set to false to enable insertion of other supported blocks.

Why?

It is counter-intuitive to insert a custom link, and then transform it into a Button, instead of adding a Button just how one would using the default inserter. This fix reduces confusion and improves user experience.

How?

Testing Instructions

In trunk

  1. Add a Navigation Block
  2. Add custom link to the Nav block.
  3. Click the inserter within the Nav Block and see you don't get the option to add the Buttons block.
  4. Switch to the fix branch and repeat 1, 2 and 3
  5. Observe you get the default inserter to insert the Buttons Block.

Screenshots or screencast

Screen.Recording.2023-10-06.at.6.37.55.PM.mov

@alexstine
Copy link
Contributor

Why would you need to add a button to the navigation block in the first place? Just wondering. I forget, does it output an actual <button> or <a>?

@hanneslsm
Copy link

Why would you need to add a button to the navigation block in the first place? Just wondering. I forget, does it output an actual or ?

Just visuals, I guess. Like "Sign up now".
The output is <a>

<div class="wp-block-button">
<a class="wp-block-button__link wp-element-button">Sign up now</a>
</div>

@alexstine alexstine requested a review from getdave October 8, 2023 05:10
@alexstine alexstine added [Type] Enhancement A suggestion for improvement. [Package] Block library /packages/block-library [Block] Navigation Affects the Navigation Block labels Oct 8, 2023
Copy link
Contributor

@getdave getdave 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 the PR. At first glance the motivation seems solid.

However, I know we need to be pretty careful with amending the values in this PR so leaving a blocking review until I have chance to review in greater depth.

@t-hamano
Copy link
Contributor

t-hamano commented Oct 9, 2023

I just wanted to add that #55144 has also been submitted, which takes a slightly different approach to this PR.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

It is my opinion that if we want to add this feature then #55144 is the way to go.

Thank you for the PR but I think we should close it out 🙏

@Sidsector9
Copy link
Contributor Author

Closing as discussed.

@Sidsector9 Sidsector9 closed this Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants