Improve SiteIcon display and transition #58472
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
|
Size Change: -8.55 kB (-1%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
richtabor
added
[Feature] Site Editor
|
Flaky tests detected in 4fe6148. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7717179051
|
t-hamano
reviewed
Jan 31, 2024
There was a problem hiding this comment.
Thanks for the PR!
I'd like to ask @jasmussen and @jameskoster for their feedback on the animation as a whole, but I'd like to leave some comments on the code level.
| @@ -36,7 +36,7 @@ function SiteIcon( { className } ) { | |||
| ) : ( | |||
| <Icon | |||
| className="edit-site-site-icon__icon" | |||
| size="48px" | |||
| size="60px" | |||
There was a problem hiding this comment.
It might be more readable to define this value in an SCSS file (packages/edit-site/src/components/site-icon/style.scss).
Perhaps the value can be expressed in $grid-unit-70 + $grid-unit-05 ?
There was a problem hiding this comment.
I think size is fine here in the Icon but it shouldn't be a px value. Fixed in 839d198.
|
Is there an issue related to this PR? I know that @afercia did extensive testing on this. |
|
@carolinan, probably #57813. @richtabor, it would be nice to make the icon display consistent between editors. |
|
Thanks for the ping. While I do understand the good intent of this pull request, I'm not sure submitting a pull request without preceding it with a related issue to allow some broader discussion is a correct process. This isn't limited to this specific pull request. There have been several cases of pull requests with no accompanying dedicated issue. I will consider whether it is worth it to open a GitHub discussion to recap why allowing broader discussion in an open source project is important. For now, I'll just point to the Contributing Guidelines of the Gutenberg project, where it is stated:
To me, 'trivial' pull requests are the ones to fix typos, adjust comments, improve documentation and such. Any other kind of pull request should have an accompanying issue. Specifically to this pull request, there's some relevant feedback on #57813 that we should think at something entirely different. |
|
Yep, let's go with this. It keeps the spirit of a meaningful difference between the two states, especially in the fullscreen state, but it fixes the problematic aspects of the implementation, notably around non-square aspect ratios, and the WordPress logo becoming huge when there's no site logo. 👍 👍 |
Regardless of the feedback on your issue, we should strive to fix what's in trunk. I'm not proposing to change any logic, but rather to resolve UI inconsistencies that need to be cleared. |
You can consider your issue #57813 as related. I'm resolving obvious design quirks (metrics really)—not proposing big changes. I don't agree that we should remove the site logo, but I do agree these metrics need cleaning up (I'm sure we all do). |
Sure, but the point is that the Site Icon is under discussion right now. To me, the Site Icon should be entirely removed. No objections to the change in this PR, just noting that as reported in #57813 it's not at all clear how to exti the editors and we should think at something entirely different so the work on this PR is appreciated but it could be pointless when / if we remove the Site Icon. |
I'm not sure keeping ignoring users feedback helps improving the user interface. |
We fix issues while discussing the future. If I can make the next version of WordPress that much nicer in a few minutes, I'm ok putting in the time. |
I'm not ignoring. I actually hadn't seen that issue before yesterday, though I do have thoughts I'll share in response (on the issue) as to why I think the site logo is important. |
|
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 Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
priethor
approved these changes
Feb 6, 2024
There was a problem hiding this comment.
The changes look good to me. I realize there is a discussion about showing the SiteIcon itself or not, but I'm focusing on this comment by @richtabor :
I'm not proposing to change any logic, but rather to resolve UI inconsistencies that need to be cleared.
Therefore, let's apply this change that improves the current UI and explore separately what to do with the SiteIcon. For example, if we end up showing or hiding it depending on a setting, this improvement will still make a lot of sense.
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?
Testing Instructions
Screenshots or screencast
Proposed:
Without site icon:
without-site-logo.mp4
With site icon:
with-logo-new.mp4
Current:
Without site icon:
old-no-icon.mp4
With site icon:
site-logo-odl.mp4