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: Cover image tooltips for alignment are duplicated #11263

Conversation

jorgefilipecosta
Copy link
Member

Description

Fixes: #6013

The same tooltip as used for Block Alignment and text Alignment.
This PR makes sure we use different tooltips for the text alignment buttons.
https://user-images.githubusercontent.com/253067/38373204-92586b58-38e8-11e8-8ec2-09e73368cacb.png

How has this been tested?

I added a cover block and I checked that the tooltips for text alignment are in the format "Text align left/center/right" and block alignment tooltips are in the format "Text left/center/right"

Screenshots

Before:
align
align2

After:
image

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Copy Issues or PRs that need copy editing assistance labels Oct 30, 2018
@ZebulanStanphill
Copy link
Member

Shouldn't this change be implemented for all text alignment controls?

@jorgefilipecosta jorgefilipecosta added this to the WordPress 5.0 milestone Oct 31, 2018
@gziolo gziolo modified the milestones: WordPress 5.0, 4.3 Nov 5, 2018
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

Nice! Works well in my testing. My only blocking comment is the copy change.

@@ -35,19 +35,19 @@ const ALIGNMENT_CONTROLS = [
},
];

export function AlignmentToolbar( { isCollapsed, value, onChange } ) {
export function AlignmentToolbar( { isCollapsed, value, onChange, alignmentControls = ALIGNMENT_CONTROLS } ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we we add a unit test for this new prop? I see that the component already has some unit tests.

Also, a nit: Renaming the constant to DEFAULT_ALIGNMENT_CONTROLS would better convey what it now does.

Copy link
Member

Choose a reason for hiding this comment

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

Was thinking we should document this new prop as well. Sadly, it looks like AlignmentToolbar is missing a README...

},
{
icon: 'editor-alignright',
title: __( 'Text align right' ),
Copy link
Member

Choose a reason for hiding this comment

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

These should read Align text left, Align text center, and Align text right.

@noisysocks
Copy link
Member

Shouldn't this change be implemented for all text alignment controls?

I think Cover is the only block that has both block alignment and text alignment in the same toolbar.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/Cover-image-tooltips-for-alignment-are-duplicated branch from a5d642f to bec8ba6 Compare November 6, 2018 21:45
@youknowriad
Copy link
Contributor

Agreed with @ZebulanStanphill I also think it'd be better if it's implemented globally for consistency. Even if Cover Image is the only one having both, it feels like the tooltip shouldn't be different across blocks.

Let's get some designer eyes here for ideas @alexislloyd @jasmussen @karmatosed

@jasmussen
Copy link
Contributor

I like the idea of both distinct tooltips (because their behaviors differ), and consistency across all text alignments.

I have a preference for "Align text left" over "Text align left", though.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/Cover-image-tooltips-for-alignment-are-duplicated branch from bec8ba6 to ee511b2 Compare November 7, 2018 13:05
@jorgefilipecosta jorgefilipecosta force-pushed the fix/Cover-image-tooltips-for-alignment-are-duplicated branch from ee511b2 to 1a98606 Compare November 7, 2018 14:27
align: 'right',
},
];

export function AlignmentToolbar( { isCollapsed, value, onChange } ) {
export function AlignmentToolbar( { isCollapsed, value, onChange, alignmentControls = DEFAULT_ALIGNMENT_CONTROLS } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not needed anymore right? can we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not needed but I liked this change, it allows a block for example to only offer right or left alignment, or to change labels in a specific case, e.g: instead of text naming the actual field "Author name align left" that may be very useful for a11y.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Copy Issues or PRs that need copy editing assistance [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants