Skip to content

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

Closed
Sidsector9 wants to merge 6 commits intoWordPress:trunkfrom
Sidsector9:fix/54877
Closed

fix/54877: Allow insertion of Buttons in the Navigation Block#55126
Sidsector9 wants to merge 6 commits intoWordPress:trunkfrom
Sidsector9:fix/54877

Conversation

@Sidsector9
Copy link
Copy Markdown
Contributor

@Sidsector9 Sidsector9 commented Oct 6, 2023

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

@Sidsector9 Sidsector9 mentioned this pull request Oct 6, 2023
Closed
@alexstine
Copy link
Copy Markdown
Contributor

alexstine commented Oct 6, 2023

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
Copy Markdown

hanneslsm commented Oct 7, 2023

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
getdave
getdave requested changes Oct 9, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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.

getdave
getdave reviewed Oct 11, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Sidsector9 commented Oct 17, 2023

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

Reviewers

@getdave getdave getdave requested changes

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

Assignees

@Sidsector9 Sidsector9

Labels

[Block] Navigation Affects the Navigation Block [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Sidsector9 @alexstine @hanneslsm @t-hamano @getdave