Move HeadinglevelDropdown to its own component #46003
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
carolinan
changed the title
|
Size Change: -752 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
carolinan
commented
Nov 23, 2022
packages/block-editor/src/components/block-tag-selection/index.js
Outdated
Show resolved
Hide resolved
carolinan
added
[Type] Enhancement
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
carolinan
commented
Nov 24, 2022
| import { useCanEditEntity } from '../utils/hooks'; | ||
|
|
||
| export default function PostTitleEdit( { | ||
| attributes: { level, textAlign, isLink, rel, linkTarget }, | ||
| setAttributes, | ||
| context: { postType, postId, queryId }, | ||
| } ) { | ||
| const TagName = 0 === level ? 'p' : 'h' + level; |
There was a problem hiding this comment.
I noticed that this code seems to have been adapted for allowing the post title to be a paragraph, but I don't remember this being implemented? It was only implemented for the site title.
Because of the discussions in this issue #30549
I have removed the partial support for p tag.
carolinan
commented
Nov 24, 2022
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Another reason for adding a new component for this is that developers would like to choose which tags are available, either via theme.json or via a filter. |
Mamaduka
reviewed
Jun 14, 2023
packages/block-editor/src/components/block-heading-level-dropdown/heading-level-icon.js
Outdated
Show resolved
Hide resolved
Mamaduka
reviewed
Jun 14, 2023
packages/block-editor/src/components/block-heading-level-dropdown/index.js
Outdated
Show resolved
Hide resolved
Mamaduka
reviewed
Jun 14, 2023
packages/block-editor/src/components/block-heading-level-dropdown/README.md
Show resolved
Hide resolved
Mamaduka
approved these changes
Jun 15, 2023
There was a problem hiding this comment.
Thank you, @carolinan! The changes test really well for me.
If there are no blockers for native/mobile, then let's merge this. cc @geriux
|
And thank you for the review. |
|
@WordPress/gutenberg-design I feel that this is ready to be merged, but I also think it could be improved if the text "Heading 1" etc, was always visible to the right of the icon inside the dropdown and not only as a tooltip, as discussed in #50402 Like in this earlier screenshot of the site title |
I think it seems a bit redundant to have both. |
|
I did not mean using both tooltip and permanently visible text. I meant the permanent text is an improvement over the tooltip, which covers the other options because of its position. |
|
Right, I mean having "H1" and "Heading 1" right next to it seems a bit redundant. I think the tooltip is performing as expected, if you move to another item, the tooltip moves with it. Is the tooltip only necessary because we don't have visible text, like in other toolbar menus? |
This was referenced Jun 27, 2023
This was referenced Jun 28, 2023
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Add a tag selection component for the block toolbar (replaces HeadingLevelDropdown) * Update index.php * update fixtures * Update CSS class, replace sprintf in the label with the tag name * Add visible tag name to the drop down, add placeholder icon * Update README.md * Try to fix issue with the deprecation * Update CHANGELOG.md * Simplify and rename component * Try reverting fixture changes * And again... * Update Query title to use the new component * Update Comments title * Update Site title with paragraph exception * Update Heading block * Update rich-text.test.js * Block Heading Dropdown - Add support for mobile (WordPress#47967) * Text changes for a native file and e2e test spec * Use the updated headingLevel icons * Update prop names * Add 'options' property to type definiton --------- Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
I'd kindly disagree. The tooltip is an usability problem now that the dropdown is vertical, as reported in #55927 and #50402. I don't think keeping both the icon and the visible text is redundant. Instead, it bring sin best of both worlds: a visual hint and an universally understandable visible text. It's not uncommon for many big applications around to display both an icon and visible text. I would also like to remind that icon-only buttons have inherent usability and accessibility problems. Their usage shoudl be limited to only cases where there's not enough space to use visible text. |
This was referenced Nov 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Updated May 24.
The HeadingLevelDropdown is a toolbar option used to choose between H1-H6 in the heading block, comments title, query title, and post title. The files are in the heading block, and the other blocks import them.
The site title has its own similar option, and it adds a paragraph to the selection.
This PR moves the HeadingLevelDropdown from the heading block to its own component.
Fixes #45743
Why?
The same code is reused in multiple blocks.
How?
Creates a new component that can be consistently reused in multiple blocks.
Testing Instructions
Screenshots or screencast
Heading block, before (The H1 is hovered over):
After, using the updated icons (The H6 is hovered over):
