-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Admin bar: Replace home/odometer dashicon with actual site icon if set #11781
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
Open
fushar
wants to merge
9
commits into
WordPress:trunk
Choose a base branch
from
fushar:admin-bar-site-icon
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
2723076
Admin bar: show site icon if set
fushar 6d09581
Update border-radius to 2px on desktop resolution
fushar 0975127
Fix multisite behavior
fushar e6dece6
Make CSS centering consistent with avatar
fushar 2b4e5d8
Use has_site_icon; fold one level
fushar 7624e1b
Update img logic
fushar d803c05
Add comment on background color choice
fushar 88aef4e
Use existing image size constants
fushar 08123a7
Fix test
fushar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this also round the blavatars in the "My Sites" menu? Is that intended and has it been discussed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's intentional (from my side) but we never actually discussed it because we just discovered that case now 😄
I think it's fine and consistent though. Even before this change, if you go to Settings -> General, and upload a site icon, somehow we already round it in the preview:
cc-ing: @jasmussen and @lucasmendes-design just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also when cropping the image from media library; here it says "As an app icon and browser icon", already suggesting the in-app icon is rounded and browser icon (favicon) is not:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the browser, the icon has rounded corners only if you upload an image with that. Otherwise, it's like what Ashar described. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the strongest reasons for why I firmly stand by my proposal that the user avatar should also be circular. We are working with images that are 20x20px in size, people of low vision need more than the contents inside for them to indicate concepts. If we establish that sites are round-rects, and users are circles, we provide a clean and clear silhouette that is immediately accessible way to differentiating between the two.