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

dev/core#378 Determine front end pages in drupal #14876

Merged
merged 1 commit into from Jul 24, 2019

Conversation

@eileenmcnaughton
Copy link
Contributor

commented Jul 24, 2019

Overview

Alternative to https://github.com/civicrm/civicrm-drupal/pull/546/files that I think narrows the scope
since it means we are not changing an existing property

Before

Cannot assign to front end pages in drupal

After

Can

Technical Details

Per https://lab.civicrm.org/dev/core/issues/378#note_20974 in 5.16 it's possible to designate front end page themes in Joomla! and WP but not drupal. @seamuslee001 proposed civicrm/civicrm-drupal#546 to address that. However, it altered the config variable userFrameworkFrontend for drupal & as @totten pointed out that is used for other things & might be unpredictable.

This gets around that by adding a function to retrieve whether it is a front end page which accesses the variable for Joomla! & WP but uses @seamuslee001's method for Drupal.

I don't know if there are any special concerns for D8 -but they are mitigated by the fact that front end themes are entirely new & will only affect those adjusting to the new setting.

Comments

I have targeted the 5.16rc as both the change this extends & the current shoreditch work target 5.16
(fyi @kcristiano )

Determine front end pages in drupal
Alternative to https://github.com/civicrm/civicrm-drupal/pull/546/files that I think narrows the scope
since it means we are not changing an existing property
@civibot

This comment has been minimized.

Copy link

commented Jul 24, 2019

(Standard links)

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Oh - the checkbox is still hidden for drupal

    {if $config->userSystem->is_drupal EQ '1'}
      <tr class="crm-preferences-display-form-block-theme">
        <td class="label">{ts}Theme{/ts} {help id="theme"}</td>
        <td>{$form.theme_backend.html}</td>
      </tr>
    {else}
      <tr class="crm-preferences-display-form-block-theme_backend">
        <td class="label">{$form.theme_backend.label} {help id="theme_backend"}</td>
        <td>{$form.theme_backend.html}</td>
      </tr>
      <tr class="crm-preferences-display-form-block-theme_frontend">
        <td class="label">{$form.theme_frontend.label} {help id="theme_frontend"}</td>
        <td>{$form.theme_frontend.html}</td>
      </tr>
      {/if}

We can worry about that if this is agreed

@seamuslee001

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I think this looks good from my POV

@totten

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Agree, this draft looks a bit safer/more-incremental than the previous draft.

@totten totten merged commit 81144b6 into civicrm:5.16 Jul 24, 2019

1 check passed

default Build finished.
Details

@eileenmcnaughton eileenmcnaughton deleted the eileenmcnaughton:is_front branch Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.