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

Uniform Focal point labels #62438

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

akasunil
Copy link
Member

What?

Fixes #62408

Testing Instructions

  • Add group block
  • Open style setting
  • Add background image
  • look for focal point control

Screenshots or screencast

image

@akasunil akasunil requested a review from ellatrix as a code owner June 10, 2024 05:35
Copy link

github-actions bot commented Jun 10, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: akasunil <sunil25393@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: jennydupuy <jdy68@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@akasunil akasunil added [Block] Group Affects the Group Block (and row, stack and grid variants) [Type] Enhancement A suggestion for improvement. labels Jun 10, 2024
@akasunil akasunil self-assigned this Jun 10, 2024
@akasunil akasunil requested a review from t-hamano June 25, 2024 11:04
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

In my feeling, the term "Focal Point" itself seems to be a bit confusing. The background panel controls four things: background-image, background-position, background-repeat, and background-size, but since the FocalPointPicker component controls background-position, it feels easier to understand as it is now.

Therefore, I would lean towards changing the label "Focal Point" to something that better matches the context.

Currently, the label "Focal Point" is used in the Cover block and Media & Text block. In these two cases, I would prefer to change the label as follows:

Media & Text Block

Focal point to Media position

media-text

Cover Block

Focal point to Image position

cover

cc @WordPress/gutenberg-design @ramonjd Please let us know your thoughts.

@t-hamano t-hamano added the Needs Design Feedback Needs general design feedback. label Jun 27, 2024
@jasmussen
Copy link
Contributor

I could go either way on this one. On the one hand, I appreciate how focal point seems to work well for the visual below with the drag handle. On the other hand, I'm leaning towards position, since top/left position values are shown right below.

@jameskoster
Copy link
Contributor

'Position' feels more universally recognisable to me.

The prefix 'Media position' is mostly useful in the Media & Text block since 'Position' would be an ambiguous option in the tools panel. But I don't know that we need that prefix elsewhere.

@ramonjd
Copy link
Member

ramonjd commented Jun 27, 2024

Thanks for the PR. I agree the labels should be the same, or similar.

I think "Position" is valid and universal label for instances of the control. I'd argue even terser and more straight forward than "focal point".

It also relates directly to the CSS property it updates (in the case of the cover block object-position) - though the tool does give the impression of setting the image origin.

@richtabor
Copy link
Member

Why not update the background image control to use the existing focal point term? Has there been feedback that "Focal point" is not clear? It's certainly clearer than "Position", in my opinion.

@akasunil
Copy link
Member Author

Thank you for the input, everyone. I appreciate the different perspectives on this. I agree that consistency across labels is important for user understanding and ease of use.

"Focal Point" is visually descriptive, it seems to cause some confusion about what it controls. "Position" might simplify user understanding, especially for those who are familiar with CSS properties ( as @ramonjd mentioned ). I agree with @t-hamano suggestions.

let me know your thoughts.

@richtabor
Copy link
Member

I don't prefer "Position" for these two reasons:

  1. Position is used elsewhere in the UI for setting CSS position, like absolute positioning.

  2. Position is less clear than focal point. The ideal behind "focal point" is that you're deciding where the focus is on this image. The "Position" label is indicative of you deciding the position of the focal point. Why abstract further from the actual intent?

@t-hamano
Copy link
Contributor

t-hamano commented Jul 5, 2024

I noticed that the background image UI is a popover in the latest Gutenberg. In this case, which is better, "Position" or "Focal point"?

image

@ramonjd
Copy link
Member

ramonjd commented Jul 5, 2024

I noticed that the background image UI is a popover in the latest Gutenberg. In this case, which is better, "Position" or "Focal point"?

I think it should be consistent, whether one or the other. As this PR stands, changing it to Focal point is 👍🏻

Just needs a rebase after #60151 as mentioned above

@t-hamano t-hamano self-requested a review July 5, 2024 14:38
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Ok, let's ship this PR 👍

@akasunil akasunil merged commit 99798ec into WordPress:trunk Jul 5, 2024
61 checks passed
@akasunil akasunil deleted the fix/uniform-focal-point-labels branch July 5, 2024 15:40
@github-actions github-actions bot added this to the Gutenberg 18.8 milestone Jul 5, 2024
@andrewserong andrewserong added the [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi label Jul 9, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jul 18, 2024
Co-authored-by: akasunil <sunil25393@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: jennydupuy <jdy68@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block (and row, stack and grid variants) [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uniform Focal point labels
7 participants