-
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
Add additional embed icons #9201
Conversation
Travis failure looks unrelated! |
Pinging @jasmussen for a review on this too since I know you've been working on icons, and also I know @mtias is super busy :) Would love to get this one in and open a separate PR for the other icons. These were all the easiest ones to find and I don't want the PR to get stale while I go hunting for the other icons. |
Thank you for working on this. Your PRs are super welcome. I've perhaps noted this in the past, I have a personal aversion to us (the open source community) becoming the maintainers of brand guidelines for third party blocks that are included. However given the user experience does make more sense without generic icons, and given those embeds are already on a whitelist, we might as well lean into it. Also, I appreciate the fact that we can include these as inline SVGs instead of having to canonize them into an icon set which automatically makes them more unmaintainable. Some questions:
Overall, since we are essentially holding 3rd parties in our hands here, I personally think we should use minimal creativity. But I also recognize that I'm biased and I'm noting non-issues here. For that reason, I've widened the review range a bit :D I think this PR will slide right in despite my thoughts, and I wouldn't object to that, to be clear! |
I agree with all of this :)
I'm not sure I understand here but is it possibly referencing some of the icons being slightly blurry? Totally fair point (SpeakerDeck jumps out at me in particular). I did my best to align the icons to a pixel grid but admittedly it's not perfect; my hope is that these can be iterated on over time by someone more skilled with the nuance of SVG/icon preparation, or have the need obviated by HiDPI.
I think falling back to the favicon, when available, is reasonable. If the service does not have an obvious icon or a favicon… well, I think that's something that will need to be evaluated on a case-by-case basis.
This is a really excellent point, and maybe worth getting a legal review on, to be honest. Are there any cases where WordPress core includes third party icons today? (I can't think of any.) What about Jetpack — how do they handle it? This could have a LICENSE implication too. |
Also FWIW this is no longer merge-ready because it needs the a11y attributes added (and I don't know that it would cleanly merge anymore because a few other PRs touching this file have been merged in). I will address that this week! |
I'm happy to revisit this but I could use a bit of clarity on whether this is still a problem we want to solve, or would rather get the third parties involved to maintain their own icons somehow… |
I don't think it's realistic to demand/expect third-parties to maintain their icons. It'd be great, and it's an open-source project where they certainly can submit fixes should they care to, but that's no excuse for not being proactive about it. I think we should solve this problem 👍 We should add the icons; if someone notices one is wrong they'll correct us, hopefully 😄 |
@tofumatt I tend to agree. I think the only potential concern is the legal aspect but we already do it for a handful of brands and I assume there's no formal agreement in those cases anyway. In any case I'll give this a refresh this week 🎉 |
I'm going to close this out — it would take a fair amount of manual labor to rebase this and also my icons weren't too great tbh. Someone else who's better at SVG prep (looking at you, @field2) could probably do a better job of this. |
Description
This PR adds additional embed icons. Goes part of the way to addressing #9124.
Here's the full list of missing icons, with the ones this PR provides checked off:
How has this been tested?
Visual testing
Screenshots
(I blurred out existing icons)
(I don't love the TED icon but the 20x20 grid is tough to work with for something like that.)Types of changes
New icons
Checklist: