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.
Open Sans is included in Panopoly by default. Should it be conditional or somehow restricted only to the back end?
Comment | File | Size | Author |
---|---|---|---|
#29 | sans-serif-subtype.png | 211.29 KB | mglaman |
#29 | sans-serif-landing.png | 125.46 KB | mglaman |
#29 | open-sans-subtype.png | 211.54 KB | mglaman |
#29 | open-sans-landing.png | 124.93 KB | mglaman |
#24 | issue-2063541.patch | 5.37 KB | lsolesen |
Comments
Comment #1
mpotter CreditAttribution: mpotter commentedSeems like this should be in the Theme layer somewhere. Perhaps explain more how this font is used and why it's hardcoded in Panopoly? Wouldn't other themes potentially use different fonts?
Comment #2
designar CreditAttribution: designar commentedThe according css is loaded for anonymous users, too.
I've removed it with a css unset for them, because we are using another font and it's a needless request.
+1 for making this part of the admin experience and configurable.
Comment #3
kmontyI support making this conditional as well. You would even need to remove Open Sans from the CSS itself, as the browser will just use the next font in the CSS-defined font-family stack. All you need to do is make loading the @font-face conditional optional.
Comment #4
lsolesen CreditAttribution: lsolesen commentedCan anybody tell where this is hardcoded in Panopoly?
Comment #5
jyve CreditAttribution: jyve commentedThis is in panopoly_core_page_build() in panopoly_core.module.
Comment #6
kmontyComment #7
kmontyComment #8
lsolesen CreditAttribution: lsolesen commentedDare we just remove the code:
This should really be in some kind of theme layer - and is there to fix issues with IE6-8. I am voting for removing it.
Comment #9
mglamanPatched worked, and I believe that this should be applied to Panopoly Core. Should be up to theme developers to handle fonts and any cross-browser rendering issues.
Comment #10
kmontyThe patch isn't making it conditional, rather, just removes it outright.
The patch would be better suited (in my opinion) if we added a on/off form element in Panopoly's admin UI and they conditionally loaded the script in hook_page_build based on that value.
Comment #11
dsnopekIn my opinion, I don't think it makes sense to have a conditional option for this - it should just be removed.
If someone writes a theme that depends on Open Sans from panopoly_core and then it's disabled - it could get messed up. Or what if the site uses two different themes in different situations, and one depends on Open Sans and the other is broken by Open Sans?
The inclusion of fonts should be handled via the theme itself OR the site builder via a module like fontyourface (which can manage some pretty complex configurations). Personally, I think Panopoly should stay out of it!
Comment #12
lsolesen CreditAttribution: lsolesen commentedThink I just saw a css file that should also be removed. Cannot remember exactly where, but will find it again.
Comment #13
lsolesen CreditAttribution: lsolesen commentedI think it should be removed - or at least moved from core to theme.
http://drupalcode.org/project/panopoly_core.git/blob/refs/heads/7.x-1.x:... --> font.css included
http://drupalcode.org/project/panopoly_core.git/tree/refs/heads/7.x-1.x:... --> font directory included
http://drupalcode.org/project/panopoly_core.git/tree/refs/heads/7.x-1.x:... --> font css files
Comment #14
dsnopekComment #15
dsnopekComment #16
kmontyMy major argument against removing it outright is Panopoly specifically markets itself as improving the administration UI--typography is absolutely a part of that.
Making it conditional puts minimal burden on site itself, without overly complicating the codebase. Thanks to font stacks in the css files themselves, not having Open Sans enabled will simply mean the browser will default to the next available font in the stack.
Comment #17
dsnopekIf the goal is to improve the admin UI, we could do that by bundling an admin theme that has Open Sans? Really, there's nothing wrong with Open Sans itself, but it'd be easier to deal with if it were added via a theme..
Comment #18
caschbre CreditAttribution: caschbre commentedI agree that the distro / modules shouldn't provide the font. I think when we get to including Radix we could eventually offer an improved admin interface and that theme handles the font.
Comment #19
lsolesen CreditAttribution: lsolesen commentedRemoved everything.
Comment #20
caschbre CreditAttribution: caschbre commentedthat patch looks like it has a lot of other changes in it as well. Was it made against the latest dev release?
Comment #21
joelpittetalso @lsolesen try
git config core.fileMode false
to avoid permission changes ending up in the patch.http://stackoverflow.com/questions/1580596/how-do-i-make-git-ignore-mode...
Comment #22
kmontyEchoing what #20 said. Seems patch was not rolled against dev.
Comment #23
lsolesen CreditAttribution: lsolesen commentedSomething strange indeed. Think i forgot to merge branches.
Comment #24
lsolesen CreditAttribution: lsolesen commentedNow without other noise.
Comment #25
lsolesen CreditAttribution: lsolesen commentedComment #26
MacMladen CreditAttribution: MacMladen commentedAlso I do not think it should be included and it is not crucial part of any experience.
Should it be part of the Admin section then it should be in some customized Admin theme that does or does not rely on some other theme (Radix or any other). There are two concepts with that, making everything larger (for mobile experience to be able to use) and smaller (for laptop/desktop to be able to fit more information).
I think Admin theme should be on its own OR if some theme is going to be part of Panopoly (Radix as it seems planned) then lets base on it.
On the other hand, I'm not sure how much energy should go in there and how much in everything else, in particular more on Panels system integration like Panels everywhere and also quality of Widgets as they really add much more to overall experience.
I do oppose to having some conditional as that really makes little sense - make Panopoly much more SASS and leave that to SASS part of building. You cannot have powerful base and babysit users.
Comment #27
populist CreditAttribution: populist commentedMy primary concern with removing Open Sans is that it will change how the Panels IPE modals look a little bit. This may not be a huge deal, but we should test to see the design effect on all of that.
Comment #28
joelpittetYeah we should take a screenshot of the before and after of a few elements and make any line-height or other changes if necessary.
Though the/a admin theme /should/ be dealing with these style changes.
Tagging as such.
Comment #29
mglamanHere are some screenshots. Plus one for this because it ruins an admin theme I'm working on based on Material Design which utilizes Roboto.
Open Sans.
Replaced with just sans-serif.
Comment #30
mglamanRemoving "Need Screenshots" tag.
Comment #31
lsolesen CreditAttribution: lsolesen commented@mglaman What does the screenshots show? Any difference?
Comment #32
dsnopekThe 'sans-serif' font on @mglaman's computer has a little less weight, and so appears a little smaller. But personally, I think it still looks pretty good! @populist: What do you think?
Comment #33
mglamanYeah it was just a weight issue. However my concern style is within panopoly-modal.css in Widgets (I say make a "clean up CSS, remove fonts" issue once this issue is settled/committed.) I want that gone as well, because it's one more thing a theme developer will have to fight to override.
IMO if this is such a big issue it should be moved out of modules and a new admin theme for Panopoly built or a specific one chosen.
Comment #34
lsolesen CreditAttribution: lsolesen commented@mglaman Agreed :)
Comment #35
dsnopekScreenshots seem perfectly fine to me, thanks @mglaman for making them! I did some of my own testing on the patch, and everything seemed totally acceptable to me. Committed!