-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Improve display of LinkURL menu and fix spacing. #33652
Conversation
Size Change: +1.77 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
There's also a little inconsistency with how the svg is rendered in the list compared to the header (middle or top). |
I've given the fix it a try, and it works great. |
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.
Thanks for looking at these visual improvements. It's really tricky to get this right so the more 👀 the better.
Unfortunately, when I tested this the loading state for the icon was visually distorted.
Screen.Capture.on.2021-07-23.at.17-49-05.mov
Also the icon now looks "low quality". It was intentionally clamped to 18px to ensure that the majority of icons (which are typically 32px) appear to be at a higher resolution.
Removing this change causes icons to look low quality which is something @javierarce was specifically against in the original review. I agree with him that it doesn't look good.
Note we can get clever and try and parse a higher quality icon but that is a larger change that will take longer and is also less reliable as not all sites provide these alternative icon files.
Ah, we can reduce the images, but we should not reduce the svg which should be set at 24px. |
Also I assume we meant 16px there for half the size? |
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 definitely looking better now on the loading front.
Screen.Capture.on.2021-07-28.at.14-45-05.mp4
However, it's worthing noting that because the loading UI for the icon is larger than that of the icon that is ultimately rendered there is a small "jump" when the rich data for the icon is shown:
Screen.Capture.on.2021-07-28.at.14-46-21.1.mp4
It's a small thing but probably worth correcting. I would look into this but I'm mostly AFK today.
@getdave can you take another look, please? |
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 looking great now. Thanks for bearing with me!
Screen.Capture.on.2021-07-30.at.12-56-38.1.mp4
Left a couple of nits regarding code comments to make life easier for future devs and to avoid potential regressions. In general I'm trying to be a lot more explicit in commenting the more obscure/complex elements of my code as I believe this helps other humans (it's advice I received from @hellofromtonya who is advocating for a similar method in WP Core).
img { | ||
max-width: none; | ||
width: 18px; // half of 32px to improve perceived resolution. | ||
width: 16px; |
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.
width: 16px; | |
width: 16px; // downscale image x2 improve perceived resolution of smaller favicons. |
Nit: I'd like to restore this to make it explicit as to what we are doing and why.
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.
Have we tried getting larger icons through the apple-touch-icon
property instead of going straight to favicon?
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 is on the roadmap...but you cannpt guarantee it. Therefore we still need these aa defaults for now.
max-height: 24px; | ||
flex-shrink: 0; | ||
width: 24px; | ||
display: flex; | ||
justify-content: center; |
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.
Nit: to make things easier for future devs and to avoid regressions it might be nice to add a code comment here to explain why we're doing this and what issue it addresses. Otherwise I'm concerned this would be open to accidental regression.
There were some spacing issues on the Link URL menu that would make the icon shrink:
Which is corrected here. It also restores the true width of the icon (24px which was assumed to be 32 before?):
There's also some alignment issues that this corrects (note the bottom part with the toggle is now aligned):
cc @javierarce
(The middle one still seems like it has spacing issues on the result list, but given it's there to accommodate the hover effect I think we can leave it be for now.)