Skip to content
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

Fix placeholder font in Quicksearch #14154

Merged
merged 1 commit into from
Jun 13, 2019

Conversation

pfigel
Copy link
Contributor

@pfigel pfigel commented Apr 29, 2019

Overview

This is a fairly trivial change to ensure the placeholder text shown in the Quicksearch input uses the same font as the rest of the menu.

Before

The Quicksearch placeholder font is set to "FontAwesome", causing it to fall back to the default UA font for regular text:

image

After

Civi's default font is used:

image

@civibot
Copy link

civibot bot commented Apr 29, 2019

(Standard links)

@civibot civibot bot added the master label Apr 29, 2019
@colemanw
Copy link
Member

This is cool. I wasn't aware that you can mix different fonts together like this to get icons + text.

However, this raises a bigger question for me, which is that there is no default font for the CiviCRM menubar, or really anything in CiviCRM. As far as I can tell, CiviCRM's fonts just blow in the wind whichever way the CMS theme sets them. And IMO we should fix this!

Where did you get the idea that the menu uses Veranda by default?

@pfigel
Copy link
Contributor Author

pfigel commented Apr 29, 2019

Where did you get the idea that the menu uses Veranda by default?

This is what I get for only using Civi with Drupal. 😄

Definitely gets more complicated with that in mind. I guess if we want to be consistent, we'd not enforce a specific font there, and use whatever the CMS gives us, but I don't see a straightforward way of doing that if we want to use font-family: 'FontAwesome' at the same time.

There are some bits in civicrm.css where we set font-family: Arial, Helvetica, sans-serif;, perhaps re-using that (with FontAwesome thrown in the mix) would at least be ... not more inconsistent? ... while fixing the odd "fallback to default UA" look?

Setting a default font would work as well, but that feels like a bigger change with some potential for headaches for people with custom themes.

@colemanw colemanw changed the title Fix placeholder font in Quicksearch [WIP] Fix placeholder font in Quicksearch Apr 30, 2019
@pfigel pfigel force-pushed the fix-quicksearch-font branch from 906b4a3 to 9f777a9 Compare May 15, 2019 16:24
This is a fairly trivial change to ensure the placeholder text
shown in the Quicksearch input uses a font that matches the rest
of Civi slightly better than the user agent's default serif font.
@pfigel pfigel force-pushed the fix-quicksearch-font branch from 9f777a9 to 9c504e4 Compare May 15, 2019 16:25
@pfigel pfigel changed the title [WIP] Fix placeholder font in Quicksearch Fix placeholder font in Quicksearch May 15, 2019
@pfigel
Copy link
Contributor Author

pfigel commented May 15, 2019

@colemanw I've updated this to not enforce any specific font for the placeholder, but rather the sans-serif pseudo-font. That one should at least look a little less out of place than the serif one currently used.

It should not introduce any issues for theming as any CSS that would target that specific element would've already had to overwrite the existing font-family.

@eileenmcnaughton eileenmcnaughton requested a review from colemanw May 21, 2019 07:38
@colemanw colemanw merged commit aadcf38 into civicrm:master Jun 13, 2019
@colemanw
Copy link
Member

I'm still a little troubled by the fact that civicrm has no font. But I seem to be in the minority, so let's put that debate aside for now and merge this.

@pfigel pfigel deleted the fix-quicksearch-font branch June 13, 2019 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants