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

FocalPointPicker: Apply modern styling #58459

Merged
merged 16 commits into from Feb 8, 2024
Merged

Conversation

richtabor
Copy link
Member

@richtabor richtabor commented Jan 30, 2024

What?

Testing Instructions

  1. Open a post or page.
  2. Insert a media and text block.
  3. Add an image.
  4. Open block inspector.
  5. See changes.

Screenshots or screencast

Current:

old-focal.mp4

Proposed:

updated-focal.mp4

@richtabor richtabor added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jan 30, 2024
@richtabor richtabor self-assigned this Jan 30, 2024
@richtabor richtabor requested a review from ciampo January 30, 2024 16:15
@richtabor richtabor requested a review from a team January 30, 2024 18:16
@richtabor richtabor marked this pull request as ready for review January 30, 2024 18:16
@jasmussen
Copy link
Contributor

Looks good!

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.

The changes look good to me, but might need a review from the @WordPress/gutenberg-components team just to be sure.

You will also need to update the changelog.

Copy link

github-actions bot commented Feb 5, 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.

Core SVN

Core Committers: Use this line as a base for the props when committing in SVN:

Props richtabor, wildworks, mciampini, joen.

GitHub Merge commits

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: richtabor <richtabor@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

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

@richtabor richtabor changed the title Apply modern styling to FocalPointPicker FocalPointPicker: Apply modern styling Feb 5, 2024
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.

Depending on the color of the background, the pointer becomes very difficult to see. How about restoring the pointer outline, perhaps the blur value?

This PR:

image

trunk:

image

@richtabor
Copy link
Member Author

Depending on the color of the background, the pointer becomes very difficult to see. How about restoring the pointer outline, perhaps the blur value?

I made a small adjustment here: a27697c

CleanShot 2024-02-06 at 14 36 23

@t-hamano
Copy link
Contributor

t-hamano commented Feb 7, 2024

Thank you for the update. The pointer is now very clear, but when I move the pointer over the outline, it seems like there is a lot of blurring. I don't have a strong opinion on this, but what do you think about reducing blur from 16px to 8px or 4px?

Current (blur: 16px):

image

blur: 8px
image

blur: 4px

image

@ciampo
Copy link
Contributor

ciampo commented Feb 7, 2024

I don't have any additional feedback to give on this PR, so I'll let @t-hamano given the final approval here

@richtabor
Copy link
Member Author

The pointer is now very clear, but when I move the pointer over the outline, it seems like there is a lot of blurring. I don't have a strong opinion on this, but what do you think about reducing blur from 16px to 8px or 4px?

I considered it, but I picked 16 for two reasons:

16px blur is used predominantly throughout, and could perhaps be standardized further. And second, I don't think we should allow the picker to go "off canvas", like it does now—I did not want to do that effort as part of this PR. If we don't have that conflict, we don't have the blurred edge issue.

CleanShot 2024-02-07 at 09 45 28

@t-hamano
Copy link
Contributor

t-hamano commented Feb 7, 2024

I don't think we should allow the picker to go "off canvas", like it does now—I did not want to do that effort as part of this PR. If we don't have that conflict, we don't have the blurred edge issue.

If you want to indicate a value of 0 or 100 in this component, the picker should come to the contour. Isn't it inevitable that the picker will go off canvas? 🤔

focal-point

@richtabor
Copy link
Member Author

richtabor commented Feb 7, 2024

If you want to indicate a value of 0 or 100 in this component, the picker should come to the contour. Isn't it inevitable that the picker will go off canvas? 🤔

I would think that the first 20px of the bounds would equate to the 0/100% mark (whatever keeps the picker from spilling into the rest of the UI).

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.

I would think that the first 20px of the bounds would equate to the 0/100% mark (whatever keeps the picker from spilling into the rest of the UI).

I see, I think it's okay to ship this PR if that approach is being considered in the future 👍

@richtabor richtabor merged commit a5c4fb4 into trunk Feb 8, 2024
58 checks passed
@richtabor richtabor deleted the try/improve-focal-point-picker branch February 8, 2024 17:55
@github-actions github-actions bot added this to the Gutenberg 17.7 milestone Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants