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

custom font in Drupal overridden #136

Closed
hansrossel opened this issue Jan 17, 2018 · 12 comments
Closed

custom font in Drupal overridden #136

hansrossel opened this issue Jan 17, 2018 · 12 comments
Labels

Comments

@hansrossel
Copy link

The last line of the css: {font-family:'Open Sans', 'Helvetica Neue', Helvetica, Arial, sans-serif !important}

is used with the !important on the very general body element so it is overriding the font used by Drupal on all pages. It does not seem needed as this font-family is already assigned to all Civicrm pages (on body class .page-civicrm).

@adixon
Copy link

adixon commented Jan 22, 2018

Yes, it looks like if you use the civicrm-custom.css file included in this extension your entire site will be converted to use Open Sans. This limits the usefulness of this extension, for sure, unless there's an undocumented way of overriding that. From my reading, the only way to fix it would be to setup gulp, etc. and edit a scss file somewhere.

@declercqt
Copy link

declercqt commented Mar 27, 2018

This seems to be an issue that only occurs when using the CSS aggregation in Drupal in admin/config/development/performance
One way to solve this is to implement a Drupal function that excludes the Shoreditch css files from preprocessing: https://drupal.stackexchange.com/questions/21142/how-do-i-tell-drupal-not-to-aggregate-my-css-file

I have not tested this (but having the same issue that I'm currently overriding with some CSS in a Drupal block), it would become something like this:

function module_preprocess_page(&$variables) { $variables['some_element']['#attached'] = array( 'css' => array( 'path/to/org.civicrm.shoreditch-0.1-alpha16/css/custom-civicrm.css' => array( 'preprocess' => FALSE ) ) ) }

Also mentioning #110 because it is definitely related.

@adixon
Copy link

adixon commented Mar 27, 2018

Ah, thanks for that observation - that suggests that the preprocessing is generating css that Drupal's aggregation is choking on.

@jensschuppe
Copy link

This is not limited to Drupal's CSS aggregation being active. The issue is that shoreditch's custom-civicrm.css is being loaded on every page, including non-CiviCRM pages - more precisely, everytime civicrm_initialize() is being called. This is, of course, due to shoreditch making use of CiviCRM core's customCSS option.

However, using !important is highly discouraged in the first place, especially within general selectors like body. This rule (and maybe many others - there are nearly 1500 style rules with !important!) should be reviewed, as they interfere with almost any Drupal theme or other custom CSS, which have to be adapted to "re-override" Shoreditch's !important style rules.

@christianwach
Copy link
Member

@jensschuppe I agree that Shoreditch is too aggressive in its use of !important. Moreover, it should not need to touch any DOM element outside the CiviCRM container. The CiviCRM menu is perhaps an exception, but Shoreditch's declarations should specifically target that and that alone.

@jensschuppe
Copy link

Seems like this issue and #234 are duplicates.

@christianwach I agree that Shoreditch's styles should only target CiviCRM markup, which may be tricky sometimes. Consider a Drupal block being placed into CiviCRM pages - this block should maybe not be affected by Shoreditch's styles, although it is placed inside a body.page-civicrm element.

@christianwach
Copy link
Member

Seems like this issue and #234 are duplicates.

@jensschuppe Yes, pretty much.

Consider a Drupal block being placed into CiviCRM pages - this block should maybe not be affected by Shoreditch's styles, although it is placed inside a body.page-civicrm element.

I would say that body.page-civicrm is outside Shoreditch's scope. It should be restricted to div#crm-container (plus the CiviCRM menu #civicrm-menu and #root-menu-div when viewing CiviCRM pages). CiviCRM blocks have the .block-civicrm class, so Shoreditch can target those too whilst leaving non-CiviCRM blocks unaffected.

@jensschuppe
Copy link

@christianwach There should be more thought put into which elements Shoreditch should apply styles on. Shoreditch only works reliably with Drupal's "Seven" theme (as stated in the README) because Shoreditch has selectors for Seven's markup. For example, you could not have the body background color when not applying CSS to markup outside of #crm-container.

You can't predict what markup is around the #crm-container element in any theme. For this to work cleanly, shoreditch would have to be a CMS theme instead of just custom CSS.

@christianwach
Copy link
Member

you could not have the body background color when not applying CSS to markup outside of #crm-container

@jensschuppe But surely this is the responsibility of the theme? Why would Shoreditch set this?

You can't predict what markup is around the #crm-container element in any theme.

Again, I don't understand why this would matter to Shoreditch? It should leave DOM elements outside its scope as they are.

@jensschuppe
Copy link

@christianwach Because Shoreditch claims, according to the README, to be

a theme for CiviCRM

which it isn't or can't be without the problems we encountered.

Shoreditch adds styles to DOM elements like the page title, the breadcrumb menu, which are not part of the page content, but theme markup. So, you suggest Shoreditch to do less than it currently tries to, right?

@christianwach
Copy link
Member

So, you suggest Shoreditch to do less than it currently tries to, right?

@jensschuppe Yes, exactly. Shoreditch is a theme for CiviCRM, not Drupal, Joomla or WordPress. In my opinion, when it tries to style elements that are not part of the CiviCRM container, blocks or menu then it is exceeding its scope.

AkA84 added a commit that referenced this issue Nov 14, 2018
PCHR-4356: Sync fork with latest release
@AkA84
Copy link
Contributor

AkA84 commented Apr 3, 2019

Closed by #381

@AkA84 AkA84 closed this as completed Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants