Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Based on the feedback given on: https://drupal.org/node/1192044#comment-8202049
Also, on devices with a higher resolution, like the iPad Air, the Drupal logo was blurry.
We should add in the SVG version and fallback to PNG. See #2286601: [policy] Drop support for browsers that don't support SVG
Remaining tasks
Review the latest patch (#106). If there are no issues left and everything works: RTBC. If there are any issues with the patch: add a comment and update this summary.
Issue category | Bug because all other icons/images in Drupal core are high resolution ("retina") ready. |
---|---|
Issue priority | Not critical because we would ship with a logo, it just would be optimised for "modern"/high pixel density devices. |
Unfrozen changes | Unfrozen because it only changes the Druplicon to a high resolution image (svg), just like all icons in core + adjust the css accordingly. |
Comment | File | Size | Author |
---|---|---|---|
#106 | add-svg-logos-2142653-106.patch | 37.17 KB | corbacho |
#106 | interdiff-102-106.txt | 11.23 KB | corbacho |
#103 | interdiff-101-102.txt | 619 bytes | corbacho |
#103 | add-svg-logos-2142653-102.patch | 26.75 KB | corbacho |
#101 | add-svg-logos-2142653-101.patch | 26.75 KB | LewisNyman |
Comments
Comment #1
LewisNymanComment #2
LewisNymanHere's a very basic patch. I couldn't help but notice the current png implementation in Bartik violates the branding guidelines?
Comment #3
sqndr CreditAttribution: sqndr commentedDo we need all this Adobe lines? I like the idea of adding an
svg
Druplicon!Comment #4
sqndr CreditAttribution: sqndr commentedComment #5
LewisNymanNa we should run it through svgoptim
Comment #6
lokapujyaCool! Easy to see the difference when you blow up the 2 images.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedOptimised version attached (saves about 5kb)
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedOops, that still had a thumbnail! Now only 4kb for the svg.
Comment #9
sqndr CreditAttribution: sqndr commentedYeah! Getting all of the Adobe tags out and thereby reducing the filesize to 4kb is a really good job.
Here a screenshot from Chrome using a retina display: http://cl.ly/image/1G3F422r3N02.
And here's a screenshot when zooming in Chrome using a retina display: http://cl.ly/image/3e1R2j3C0H3r.
Maybe Lewis himself can give this another review so we can get this in!
Comment #10
floretan CreditAttribution: floretan commentedThe SVG version definitely looks better. However, here are some more issues:
Comment #11
floretan CreditAttribution: floretan commentedIt turns out there's only two issues: size/alignment and using a transparent logo instead of a blue one. The attached patch addresses both issues.
Comment #12
floretan CreditAttribution: floretan commentedHere's a screenshot with a comparison between the PNG logo and the new SVG one.
Comment #13
ngocketit CreditAttribution: ngocketit commentedReviewed and tested. Looks good to me.
Comment #14
sqndr CreditAttribution: sqndr commentedLooks good to me as well. Let's wait for the testbot and get this RTBC!
Comment #16
sqndr CreditAttribution: sqndr commented11: svg-logo-2142653-11.patch queued for re-testing.
Comment #17
floretan CreditAttribution: floretan commentedNot sure why the tests failed the first time, apparently everything is fine now.
Comment #19
floretan CreditAttribution: floretan commented11: svg-logo-2142653-11.patch queued for re-testing.
Comment #20
corbacho CreditAttribution: corbacho commentedpatch applies nicely, and it works fine. It's so sharp in retina screens!
Couple of minor issues
The svg logo still looks a couple of pixels bigger than original png. So still, It's not totally aligned with the Home Tab. (both in home page and preview page)
(I don't know why I can't embed it. It's a relative url)
With white background, the druplicon doesn't look very good. Maybe adding a bit of grey shadow in the middle of druplicon would make it look better?
Comment #21
LewisNymanThanks for the feedback, this seems like a novice task
Comment #22
sqndr CreditAttribution: sqndr commented@corbacho: Could you maybe add the *.svg you've created in the screenshot above? I don't know how to create that grey shadow in the middle of druplicon? I'd be happy to create the patch, since I'd love a retina-ready druplicon. It's on the front page when Drupal is installed, so it would be really nice to get this in!
Comment #23
corbacho CreditAttribution: corbacho commented@sqndr sorry, the screenshot at #20 were comparing the old *png* with the new svg.
But I've been working on the SVG in the weekend. I will put a patch + screenshots later today :)
Comment #24
sqndr CreditAttribution: sqndr commentedAwesome! I'll happily review that one for you! ;)
Comment #25
corbacho CreditAttribution: corbacho commentedHere it is.
Aligned.
See image
And I try to balance the opacity of the areas and fill colors, to make nice gradients in all range of colors. Starting from the original Druplicon SVG, I splitted the main region in 2 areas, so they don't overlap and it's easier to control the attributes of both areas independently.
Compared to the previous logo.png
You can play with it here: http://jsfiddle.net/corbacho/ZzLfx/
The SVG was optimized with SVGO tool and now it's only 3.2Kb
About using logo.png as fallback, we don't need to support IE8, but if we want support for Android 2.3, it's quite difficult to detect it. Modernizr svg feature detect seems to be the only reliable way. See Chris Coyier post, and specially this comment.
Adding to Related -> I see an issue about Drupal 8 SVG support #2286601: [policy] Drop support for browsers that don't support SVG
Comment #26
sqndr CreditAttribution: sqndr commented@corbacho: Nice! Looks like a good icon at first sight. Don't forget to change these lines, since they are not in the patch:
Comment #27
sqndr CreditAttribution: sqndr commentedHaha, couldn't wait so wrote that patch myself. Here we go:
Comment #28
sqndr CreditAttribution: sqndr commentedSo - yeah, needs another review ;)
Comment #29
corbacho CreditAttribution: corbacho commentedTrying to make a side-by-side comparison, I discover a bit extra padding that increases the header size
Otherwise, I think this is ready folks. No PNG fallback needed, since we agreed on it #2286601: [policy] Drop support for browsers that don't support SVG
Comment #30
sqndr CreditAttribution: sqndr commentedLooks good! As mentioned by you, no fallback is needed. Awesome!
Comment #31
alexpottSo why is logo.png hanging around :)
Doesn't this break uploading of your own logo in the bartik theme?
Comment #32
sqndr CreditAttribution: sqndr commentedRight, we're not doing *.png any more, so I removed it. Will look into the other issue (
width: 57px;
), see if it's needed or not.Comment #33
sqndr CreditAttribution: sqndr commentedComment #34
sqndr CreditAttribution: sqndr commentedNeed some opinions here. If we don't include a
width
, thesvg
is massive, and takes up almost the entire screen. Since it's a vector, I supposemax-width: 100%;
isn't working ;-)Comment #35
sqndr CreditAttribution: sqndr commentedWould it be possible to save the svg with a default width of
57px
?Comment #36
corbacho CreditAttribution: corbacho commentedI agree with sqndr, there is nothing in the HTML or CSS classes to differentiate if it's the default druplicon or a custom logo. So the only way to fix it will be to hard-coded it directly on the logo.svg. SVG by default has preserveAspectRatio enabled. So no height needed.
I'm not a fan of hard-coding values, but the svg hardcoded width can be overridden via CSS, in case someone needs:
Sorry I couldn't create an interdiff.. (using the interdiff tool of patchutils), it complains about malformed patch. I think it couldn't digest those PNG strange characters.
Please, test the patch thoroughly.
PS. useful guide about SVGs coordinate systems
Comment #38
LewisNymanIt's saying it can't find logo.png?
Comment #39
corbacho CreditAttribution: corbacho commentedMhh.. Trying again. Now interdiff worked, that's a good sign.
I modified a bit the SVG colors to add more contrast. Druplicon looked too dark with the new header gradient colors committed at #2125621: Contrast between title/slogan and header is too low
Screenshot: Before #32 and After #39
Also, with different background colors: http://jsfiddle.net/GvR3X/1/
Comment #40
LewisNymanThis looks great! Sorry it took a while to get round to.
Comment #41
LewisNymanComment #42
sqndr CreditAttribution: sqndr commentedAwesome! Let's get this in so the Druplicon can shine on high resolution displays!
Comment #43
patrickd CreditAttribution: patrickd commentedcorrecting amsterdam tag (No space)
Comment #44
alexpottI still think that we've not solved the upload of custom logos - automatically scaling them to 57px will be completely unexpected.
Comment #45
LewisNymanhm, is it unexpected for the theme to control the size of the logo? That's a design decision. It's very common for a theme to control the size of images when working with SVG, especially if it's in a responsive environment when the logo might be in a fluid layout.
Setting to needs review for more opinions.
Comment #46
corbacho CreditAttribution: corbacho commentedI understand the concern of alexpott. For site-point-click-builders with no CSS knowledge, maybe they want to upload a custom logo (from theme settings), and bartik shouldn't dictate the width of the logo. In D7, it's like that.
A solution: We could use a attribute CSS3 selector
img[src^="core/themes/bartik/logo.svg"]
to check if it's the default bartik logo ?It should work on IE7+ http://www.quirksmode.org/css/selectors/
What do you think? Other solution is to check from PHP side if it's the default logo, then output some extra class to identify it. But sounds hacky
Comment #47
LoMo CreditAttribution: LoMo commentedI think this looks good. I've tested in Android and on various Mac browsers and I can see it's a nice SVG image and everything looks fine. It's senseless to keep this open to nitpick... if there is anything to further improve, that could come in a future issue, but there are bigger fish to fry right now, aren't there?
This is a screenshot from android
Comment #48
LewisNymanStill could do with a few more opinions I think. I don't think changing behaviour on the default image is a good idea, because won't this cause problems whenever a user uploads an svg?
Comment #49
Outi CreditAttribution: Outi commentedI agree that the logo shouldn't be set by default at 57 px. I tried with a custom logo I uploaded on the UI, and if I didn't know how and where I can change the CSS, I couldn't use this logo.
I wanted to test the CSS3 selector method corbacho proposed in #46, but I didn't manage to make it work.
Comment #50
BarisW CreditAttribution: BarisW commentedSo, some remarks while testing this:
- Why has the page.html.twig in Bartik still has an img tag in it? We have a site-branding block now.
- We should add a width and height to every logo, either svg or a user-uploaded one. Thats an front-end performance issue as the browser won't start redering the image unless it fully downloaded it.
- Why isn't the logo variable in the twig files an img tag, but only its path? If we keep it like this, can we at least name it logo_path? If we make it a variable, we might be able to set the width and height in theme.inc when we use the default logo.
Anyway, the concerns from #44 are not tackled by the patch in #39.
Comment #51
LewisNymanI think we got stuck choosing a solution here so I'm going to suggest the simplest one:
If we do this then we don't have to modify any of the CSS in Bartik. I've also done this in client projects, mainly because when it gets converted to a PNG it needs the initial width. We're able to manipulate the SVG width in CSS and that's what most people do anyway, so I don't think this will be a big problem in practice.
So the remaining tasks would be:
1. Remove all the CSS that sets the 57px width.
2. Set the default width and height in the SVG
Comment #52
G-raph CreditAttribution: G-raph commentedI created the svg with a width of 57px and deleted the css that sets this svg to 57px.
New patch in attach.
Here some screenshots:
Comment #53
G-raph CreditAttribution: G-raph commentedComment #54
G-raph CreditAttribution: G-raph commentedComment #55
markhalliwellThis should probably be postponed until this related issue is finalized.
Comment #56
sqndr CreditAttribution: sqndr commentedSince Dries has not decided yet and due to the fact that he's more leaning towards not changing the Druplicon, we can continue working on this issue to make sure Drupal ships with a high-res Druplicon. If the discussion in the related issue changes towards changing the Druplicon, we can close this issue and create a new issue to change the Druplicon with the new version - and make sure that it's an SVG.
Comment #57
sqndr CreditAttribution: sqndr commentedI minified the
*.svg
file using SVGO (http://cl.ly/image/3f1r1y073b3D). The rest of the patch looks good to me. Added a new patch, with the minified*.svg
file.Comment #58
BarisW CreditAttribution: BarisW commentedLooks great Sander, thanks for minifying. Here's a patch that adds the svg to Classy as well (oddly, it was missing a logo).
Comment #59
corbacho CreditAttribution: corbacho commentedarggh, sorry to deliver bad news.
The Preview functionality of Bartik settings, shows a broken image. It's trying to load still the png logo. See file /bartik/color/preview.html
Screenshot of Classy and Bartik here: http://monosnap.com/image/UI9pognUlsmgxAmOPneSibq0IWYUuq It looks perfect in retina.
A bit cleanup of preview functionality, and this is RTBC
Please, announce if you are taking this last task, so we don't step on each other.
Comment #60
BarisW CreditAttribution: BarisW commentedWill work on it right away.
Comment #61
BarisW CreditAttribution: BarisW commentedHere's a patch that fixed the logo in the Preview too. Also, I found other references to logo.png in the tests and updated these too.
By the way: good catch. Thanks for pointing that out!
Moving this to markup, as it applies to Stark, Classy and Bartik.
Comment #62
sqndr CreditAttribution: sqndr commentedComment #63
sqndr CreditAttribution: sqndr commentedComment #64
corbacho CreditAttribution: corbacho commentedThanks Baris Sander and Guter, I think we got it all right now. :)
I tested Bartik, and the preview. Saving settings with different color configurations. All good.
The logo in Stark looks good, same than screenshot of Classy (attached in #59)
Thumbs up!
Comment #65
webchickAgreed with the evaluation template in the issue summary. Additionally, from the perspective of mobile support, this is really just fixing a bug. I think it's fine to fix this now, rather than block on #2057767: [policy, no patch] Revise the Druplicon logo, since it might be awhile until that is sorted.
Looks like Alex has chimed in here a few times, so assigning to him for sign-off.
Comment #66
alexpottSo what happens in color module is still a problem. A svg gets created in sites/default/files/color/bartik-COLOR/logo.svg
But this code
Saves it in config calling it a png... and...
Expects logo to be a png but it's an svg and therefore by not working makes color work because the svg has no colour!!!
The function _color_render_images() looks suspiciously like it only supports png's.
The new svg logo is a big imporvement on a retina screen and I look forward to committing this patch but we've got more work to do on the color module.
Comment #67
corbacho CreditAttribution: corbacho commented@alexpott
Ouch, I see. I can replicate.
It seems Color module is copying & hard-coding logo paths to "files/color/theme-xxx/logo.png". Quite bad.
The history
----------
The current behaviour of Color module is due of historic reasons, when Garland in D6 had to generate **and fill** the transparencies of the logo (because of IE6 support).
Later on, when Bartik #683026: Add a new theme to D7 core: Bartik was introduced, it continued the same pattern, even it was not necessary. (the logo.png was not filled, just used as-it-is).
The solution
-----------
Get rid of "copying" the logo when creating a color combination. We don't need to. Bartik in D7 doesn't need that neither (but it continued the historic pattern)
When I got rid of it, also I saw that we can remove the config setting, and preprocess_page hook. No theme in Drupal 8 core need anymore a special logo-per-color-combination. And I believe that no contrib theme needs it neither IMHO. The browsers have changed.
Notice that this is different than overriding logo with a custom one. That is still working.
All Drupal 8 now is free of "logo.png" references :)
Comment #68
alexpottI think the solution looks ok to me. We will need a change record to outline this new behaviour.
Also looking at the code I wonder about the "copy", "base_image" and css filepath rewriting functionality. This is all completely untested. Messy. I've asked for markcarver's feedback on this as the person I know with the most expertise in the color module.
Comment #69
markhalliwellI agree with @corbacho, the whole "copying/color-fying" of logo/images is rarely used and is legacy functionality from 6.x when CSS/image techniques were... limiting to say the least; CSS3 was but just a dream.
Removing
color_preprocess_page()
is, however, technically an API change. I'm not entirely sure that removing something like this so late in the game is very wise, the color module is definitely "... all completely untested. Messy." There is a lot of legacy code I wanted to remove from color but was always pushed back with the fact that we were well past feature/bc breaking freezes.We can definitely remove the copying of the logo from bartik's color.inc. However, instead of removing
color_preprocess_page()
entirely, we should only save the color.logo configuration incolor_scheme_form_submit()
if the "logo.png" file actually exists. Then incolor_preprocess_page()
the logo should only be replaced if it has been actually been set. Which, in the case of Bartik (and most other themes I suspect), wouldn't happen.Removing the "Needs change record" tag as the above approach isn't actually changing anything, just fixing a bug in the color module (ie: url/path to logo replaced by color even when it doesn't exist). I'm sure there's a color issue for this exact thing somewhere, I just don't remember or have time to hunt it down :)
Comment #70
alexpottThanks @Mark Carver #69 makes lots of sense.
Comment #71
corbacho CreditAttribution: corbacho commentedThanks Mark! Really good suggestion
I'm on the same page. It's a pity we can't remove some legacy code. The solution you propose is sensitive.
I changed the patch. When saving the theme settings, Color module will check if the theme is using a logo.png in
['copy']
. In that case, the logo path will be pointing to the logo copied in the 'files' folder. It will override the default logo. This behavior is exactly as before.The difference with this patch, is that if there is no logo.png in
['copy']
, path won't be saved, and color module won't override the logo.I didn't have to modify
hook_preprocess_page
at all.This is a per-theme setting, so there is no concern about side-effects when user is switching themes.
I tested all possible combinations (with/without custom logo, with/without logo.png in 'copy', with default colors/custom colors) and it behaves as expected.
Comment #72
markhalliwellAwesome work @corbacho :) This is certainly moving in the right direction.
I'll admit though that I was just looking the proposed color module changes in the prior interdiff comment. However, after looking at the full patch now, I still think there definitely needs to be some more work done here.
Simply changing these from
.png
to.svg
is still an API break. These changes have nothing to do with the color module and are, in fact, part of the theme system "API". Not every contrib/custom theme will have a.svg
logo and this will break existing themes that have logo.png files.There should be logic in place to actually detect if a logo is present and which extension to use. Technically speaking, this is a feature request and as we are adding the functionality to support SVG logos in themes. We shouldn't be removing PNG logo support.
No need to use
->save()
here, it's called below.Comment #73
corbacho CreditAttribution: corbacho commentedThank you for the review Mark.
New patch supports themes with logo.png and logo.svg. (In case both are present, the logo.svg will be the default)
I think is appropriated to use by default "logo.svg" in the description of
ThemeSettingsForm
, and avoid use 'logo.png' there. See screenshot. Anyway this description is collapsed by default. And "logo.svg" is used as an example, it won't affect anything or suggests is the default value. In the moment user uploads a custom logo in that field, the description uses the custom logo filename.What matters is the values that
theme_get_setting('logo.url');
, and it now supports both png and svg.Comment #74
corbacho CreditAttribution: corbacho commentedI saw in #61 we renamed colors.css to
colors.svg
by mistake. And I've dragged that in my patches too.Corrected now. Please review!, let's give Drupal 8 a retina logo for Christmas.
Comment #75
sqndr CreditAttribution: sqndr commentedI've manually tested the patch from #74 with a Retina MacBook. The patch looks good for Bartik. In Bartik, there is a background color and so the color of the Druplicon change accordingly. Using the color module, the Druplicon will change as well. In Stark and Classy, the Druplicon is white/grey because it's displayed on a white background.
Screenshots are attached.
I'm hoping we can get there as well! Nice work corbacho!
Comment #76
Jeff Burnz CreditAttribution: Jeff Burnz commentedI assume this is a typo, no need to set $favicon.
else on a new line, coding standards.
Comment #77
corbacho CreditAttribution: corbacho commentedThank you sqndr for the manual review.
And I addressed those two issues in #76 by Jeff. Well spotted! Thank you
Comment #79
LewisNymanI tried applying the last patch manually but it said it was corrupted. I applied the patch from #74 again, made the changes Jef suggested, and made a new patch. Seems to apply.
Comment #80
corbacho CreditAttribution: corbacho commentedIgnore this patch. Argh
Comment #81
corbacho CreditAttribution: corbacho commentedgreat Lewis! that explains why I couldn't generate a clean interdiff, I had some problems.
I'm attaching a slightly different patch, see interdiff. It's exactly the same strategy than favicon file path, just some lines below.
Hopefully this is the last patch :)
Comment #82
LewisNymanIt looks like we've covered all the raised concerns here, thanks for the hard work everyone.
Comment #85
corbacho CreditAttribution: corbacho commentedtests are ok again. thanks sqndr for re-testing
Comment #86
alexpottThis is problematic - since this will result in an unnecessary file system hit for every request using the default logo.
These statements are contrary as there can never be a color version of the logo with an svg image (or a png actually - but that is an existing bug)... It might be simpler to remove this in a separate issue - I think it is - so I've created #2396789: Color no longer needs to manipulate logos to do this.
Comment #87
corbacho CreditAttribution: corbacho commented@alexpott Thanks for great review again
About 1. I see and I noticed too that
file_exists
can be expensive. But I don't see why is unnecessary. Is there any other way to know if the theme is providing a SVG or PNG logo? (without adding a new admin setting). The code is inspired on how the favicon.ico file is discovered (some lines below).On the bright side, if someone has a great idea on keeping the functionality without "file_exists", we could get better performance for the favicon handling also.
About 2. Good bug catching :) always nice to remove dead code. I'll review tomorrow
Comment #88
Jeff Burnz CreditAttribution: Jeff Burnz commenteddeclare them like we do other assets, i.e. in the info.yml file?
Comment #89
alexpottWe could always move to only supporting svg as the default logo for a theme.
Comment #90
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'd rather have the ability to decide whats right for my project, rather than Drupal dictating a single file format, we already had years of that and now we have issues with it, we can solve the problem and allow any format (even WebP if I want). Also I'd really rather not have logo, screenshot and favicon littering my theme root.
Comment #91
LewisNymanWe worked on similar code in #1507896: Allow theme developers to add the default logo filename to the theme's .info.yml, maybe it will help here.
Comment #92
alexpottSo #2396789: Color no longer needs to manipulate logos does not work out. If the logo appears in a css file the path will be rewritten :(
If we want to go down the line of
We are going to need to implement some key in the theme's info.yml - but this is feeling very feature-ish - atm we only support png logos - it feels far less risky at this point to only support svg logos. Supporting multiple logo types will take plenty of new code - but supporting only one is string replace.
Comment #93
LewisNymanRight now there is nothing to stop you defining whatever filetype theme settings. This just the default. With this in mind I think a switch from PNG to SVG is not going to cause any more problems than leaving the default file type to be PNG now.
Comment #94
Jeff Burnz CreditAttribution: Jeff Burnz commentedLets do it then, I think the objections raised in #90 are sufficiently answered. We can always discuss such things in #1507896: Allow theme developers to add the default logo filename to the theme's .info.yml
Comment #95
LewisNymanComment #96
corbacho CreditAttribution: corbacho commentedI agree on supporting only svg. But I would like to reconsider #2396789: Color no longer needs to manipulate logos, if you can have a look. So we can get rid of dead code in color module. (this shouldn't block this issue though)
Comment #97
LewisNymanHere's the reroll.
Comment #98
alexpottSo if we only support svg the file exists is not needed. Just replace png with svg
Not needed. Just replace png with svg
Replace .png with .svg
Comment #99
LewisNymanThanks Alex that was really helpful.
Comment #101
LewisNymanWhoops, the patch didn't work.
Comment #102
LewisNymanComment #103
corbacho CreditAttribution: corbacho commentedLooks good Lewis!, I reviewed the patch and tested manually.
A space needed after the . in
.....getPath() .'/logo.svg'));
I fixed that in the patch below.. but this is RTBC ! :)
I noticed another bug, not blocking this issue. Follow-up: #2396983: Header Logo with Bartik won't change in settings preview
Comment #104
Jeff Burnz CreditAttribution: Jeff Burnz commentedWe need to remove the classy png logo.
Comment #105
lauriiiClassy logo is there for reason; its being used in tests.
Edit: Sorry for messing around. There is .svg logo added already in the patch so .png only needs to be removed!
Comment #106
corbacho CreditAttribution: corbacho commentedGood catch Jeff
I removed the
logo.png
from classy and seven. And I addedlogo.svg
to seven.Seven theme doesn't output the logo in the template, as you know, but it felt wrong to leave it without logo. What do you think Lewis ?
So all core themes logo.png have been replaced with the new logo.svg
I made a search in all code base for "logo.png", 0 results.
interdiff shows also changes in
ThemeSettingsForm.php
, only because the lines have changed. (@@ Numbers part of the diff). Nothing to worryComment #107
LewisNymanWe might as well replace the Seven logo here and have the discussion of removing it in a separate issue.
Comment #108
sqndr CreditAttribution: sqndr commentedUpdated the summary for the sprints.
Comment #109
alexpottWe need a CR
Comment #110
LewisNymanPlease review this change request: https://www.drupal.org/node/2410787
Comment #111
dasjoSorry if I am late to the party for feedback. While I understand we don't need a png fallback to support old browsers #2286601: [policy] Drop support for browsers that don't support SVG i still wonder if we could have a fallback to a logo.png for theme developers. Now they need specify this via custom theme settings, if the fallback was there it would just pick up logo.svg first or logo.png if the first doesn't exist?
Other than that the change notice looks good :)
Comment #112
Jeff Burnz CreditAttribution: Jeff Burnz commented@dasjo, if you are looking for an ie8 fallback you could do this in the template with conditional comments, I actually saw this on the toyota.com site the other day, lol.
Comment #113
LewisNyman@Jeff I think Dasjo is actually requesting the patch we had before, which is a check for SVG and if not a fallback check for PNG. Like Alex said in #92 this feels like a serious feature to add at this stage. Is this something that contrib could provide if needed?
Comment #114
alexpottre #113 nope contrib can't provide this. It is baked into theme_get_setting().
Comment #115
markhalliwellAgreed. This issue has major scope creep.
Considering that theme settings are ultimately cached, could we simply not introduce an alter hook (new simpler issue) instead? This would certainly allow us to provide/flush out this functionality in contrib and then we can postpone this issue to 8.1.x.
Re-titling/categorizing since this is/should be what this issue is really about. Changing the Druplicon in Bartik should really be another separate issue dependent on this one.
Comment #116
corbacho CreditAttribution: corbacho commentedI edited a bit the CR to be more clear that core is not supporting a default PNG logo.
Concern of @dasjo is valid
@markcarver. The support of different logo types is open at this issue #1507896: Allow theme developers to add the default logo filename to the theme's .info.yml. Don't change the scope of this one.
As Alex said in #92, multiple logo types will take plenty of new code - but supporting only svg, is basically a string replace.
Support for logo.png can be achieved easily in contrib themes:
In
themename/config/themename.settings.yml
:In
themename.theme
:In page.html.yml:
I don't recommend necessarily this type of fallback, it's just an example.
You could use also override the variable "logo" inside the
hook_preprocess_page
.Conclusion
There are multiple alternatives, to provide a PNG default logo. It shouldn't block this looong issue IMO.
Comment #117
markhalliwellin three different files... that is hardly an "easy" solution.
edit: p.s. I raised the BC break back in #72 FWIW...
it was ignored, it wasn't ignored... it was just replaced again after #86 and #98.Comment #118
markhalliwellI see where this got derailed again: #86 and #98. @alexpott, yes...
file_exists()
is technically "expensive", but these settings are cached immediately after are they not? Why would that not be an appropriate place to check if there is one file type or another?Comment #119
corbacho CreditAttribution: corbacho commentedI hear you Mark, but where you see "derailed" I see consensus on a best possible solution, knowing that we are late on the game and the color module is difficult to work-around without introducing a bunch of new code.
As many have said, the "default" logo... is just that, a default. You can specify the logo you want as a custom logo, and it's exportable. This is the way all these theme assets (favicon.ico, logo.png) probably will (should) behave in the future, instead of relying on outdated conventions and expensive guessing with
file_exists
.I would like @alexpott to give his opinion here.
PS. I would appreciate Mark if you can add your opinion here, since you are familiar with color module #2396789: Color no longer needs to manipulate logos
Comment #120
LewisNymanWe are pre-RC, which means we are still able to break backwards compatibility for themes, at least I think so.
That's not completely true, SVG is the new standard and retina compatibility is an issue for every website in the next 4 years if it's not already. Every company logo has a vector equivalent. I'm more interested in supporting the many theme developers going forward then the few current contrib theme that don't want to provide the correct assets.
It's Alex's call if we should deal with this compatibility break at here or if we should deal with it in #1507896: Allow theme developers to add the default logo filename to the theme's .info.yml
Comment #121
alexpottSo the issue is that the theme settings are statically cached - so this is rebuilt on every request. So a file_exists here represents a something extra on every request. However, this should be cached by the block cache since the logo is displayed as part of the
SystemBrandingBlock
. However this cache is busted on everything other than a get request. So any request that is probably of value (a node creation, comment or purchase) to your site will result in extra disk access.Looking at
theme_get_setting
I think it is simple to add alogo.default_extension
setting to allow theme authors to do what they what to. But let's do that in #15. I agree with @LewisNyman's comment in #120 about retina compatibility being important for now and the future.Committed a2ea305 and pushed to 8.0.x. Thanks!
Thank you for adding the beta evaluation to the issue summary.
Comment #123
dasjoyay!
does this task need to be updated for the change notice now?
https://www.drupal.org/node/2410787