(accessibility/misc-core-fixes/issues/3) Remove CMS specific print header templates that share same code #11944

monishdeb · 2018-04-05T06:40:23Z

Overview

While working for https://lab.civicrm.org/accessibility/misc-core-fixes/issues/3 about making labels accessible I noticed that the files under templates/CRM/common/{UserFramework}.tpl share the same code and thus this patch replaces all those with a new file CMSPrint.tpl and contain other changes at the server end to accept the new file.

monishdeb · 2018-04-05T06:41:04Z

ping @eileenmcnaughton @seamuslee001

eileenmcnaughton · 2018-04-05T07:17:49Z

templates/CRM/common/CMSPrint.tpl

@@ -35,14 +35,14 @@
 {if isset($browserPrint) and $browserPrint}
 {* Javascript window.print link. Used for public pages where we can't do printer-friendly view. *}
 <div id="printer-friendly">
-<a href="#" onclick="window.print(); return false;" title="{ts}Print this page.{/ts}">
+<a href="#" onclick="window.print(); return false;" title="{ts}Print this page.{/ts}" aria-label="{ts}Print this page.{/ts}">


@monishdeb this has other changes in it as well though? I was hoping just to merge the file duplication clean up since I haven't formed an opinion on the other changes as yet

eileenmcnaughton · 2018-04-05T10:40:45Z

I worked through this myself doing the following

checkout master
apply this patch
revert this patch (so code is the same as master)
reset HEAD~1 - ie reset the revert but code is per master
check git is not clean :-)
a series of moves like

 mv templates/CRM/common/backdrop.tpl templates/CRM/common/CMSPrint.tpl

grep CRM/common/ & replace appropriately with the ref to the new file
check git is now clean (only one difference - I removed a now redundant $config =

So, I feel confident this is a clean amalgamation of duplicate code, merge on pass

seamuslee001 · 2018-04-05T10:45:59Z

@monishdeb @eileenmcnaughton the only code i don't think has been brought across is

{* Joomla-only container to hold the civicrm menu *}
<div id="crm-nav-menu-container"></div>
{crmNavigationMenu is_default=1}

<table border="0" cellpadding="0" cellspacing="0" id="crm-content">
  <tr>
{if $sidebarLeft}
    <td id="sidebar-left" valign="top">
        <div id="civi-sidebar-logo" style="margin: 0 0 .25em .25em"><img src="{$config-resourceBase}i/logo_words_small.png" title="{ts}CiviCRM{/ts}"/></div><div class="spacer"></div>
       {$sidebarLeft}
    </td>
{/if}
    <td id="content-right" valign="top">
    {if $breadcrumb}
    <div class="breadcrumb">
      {foreach from=$breadcrumb item=crumb key=key}
        {if $key != 0}
           &raquo;
        {/if}
        <a href="{$crumb.url}">{$crumb.title}</a>
      {/foreach}
    </div>
    {/if}
``` which is from the joomla template

eileenmcnaughton · 2018-04-05T10:53:03Z

how odd - how did I mess that? Perhaps we had better keep the files & put in

{include "CRM/common/CMSPrint.tpl"}

monishdeb · 2018-04-05T11:30:15Z

Thanks @seamuslee001 for pointing that out and made the changes in CMSPrint.tpl itself https://github.com/civicrm/civicrm-core/pull/11944/files#diff-262551485634c80891f7f7ae1e804c30R32

or should I still keep the files? @eileenmcnaughton

eileenmcnaughton · 2018-04-05T11:40:23Z

@monishdeb yeah - I think I would keep all files (but replace the contents with an include) rather than have references to a specific cms in a generic file

monishdeb · 2018-04-06T13:26:50Z

@eileenmcnaughton done, please have a look!

eileenmcnaughton · 2018-04-06T18:47:28Z

I think we still need to move the joomla specific parts back to the joomla tpl.

We wouldn't need the joomla if them either

{if $config->userFramework|lower eq 'joomla'}

Not sure if we should move the outer div back to the cms specific ones so joomla can share the inner part - perhaps

JoeMurray · 2018-05-18T15:42:46Z

@eileenmcnaughton I believe @monishdeb has updated the PR for you to review.

eileenmcnaughton · 2018-05-20T22:41:46Z

templates/CRM/common/drupal6.tpl

-</div>
-{/if}
-
-{*{include file="CRM/common/langSwitch.tpl"}*}


So this line is removed from the final tpl - but as it is commented out that seems fine

eileenmcnaughton · 2018-05-20T22:42:30Z

templates/CRM/common/wordpress.tpl

-
-{crmNavigationMenu is_default=1}
-
-{if $breadcrumb}


This chunk here is missing from the final tpl - was that deliberate?

eileenmcnaughton · 2018-05-20T22:44:27Z

templates/CRM/common/CMSPrint.tpl

@@ -0,0 +1,82 @@
+{*
+ +--------------------------------------------------------------------+
+ | CiviCRM version 4.7                                                |


Opps - we are on 5.x now

eileenmcnaughton · 2018-05-20T22:44:54Z

Thanks @JoeMurray. @monishdeb there is one piece of code in the Wordpress.tpl that is omitted relating to the breadcrumb. Can you comment on that.

Also, either this PR or a follow up PR needs to fix the version in the new tpl

…ader templates that share same code

monishdeb · 2018-05-21T06:55:09Z

templates/CRM/common/CMSPrint.tpl

+      {$crumb}
+    {/foreach}
+  </div>
+{/if}


@eileenmcnaughton thanks for notifying it, restored the breadcrumb block and ensure that this variable is exclusively available for wordpress as per

$ grep -irn "('breadcrumb'" CRM CRM/Utils/System/Joomla.php:187: $bc = $template->get_template_vars('breadcrumb'); CRM/Utils/System/Joomla.php:205: $template->assign_by_ref('breadcrumb', $bc); CRM/Utils/System/WordPress.php:147: $template->assign_by_ref('breadcrumb', $breadCrumb);

monishdeb · 2018-05-21T07:00:54Z

@eileenmcnaughton fixed both the issues, please have a look. I was earlier thinking of including this chunk of code https://github.com/civicrm/civicrm-core/blob/master/templates/CRM/common/joomla.tpl#L56L98 in CMSprint.tpl as shared by all CMS tpl which allow us to keep the CMS specific code in respective tpl and include the shared code via CMSprint.tpl

eileenmcnaughton · 2018-05-21T07:12:41Z

@monishdeb I added merge on pass as I'm happy this makes sense now - I think extracting that block to a tpl which both Joomla.tpl & CMSPrint.tpl include makes sense - is that what you mean? We can cover in a follow up PR

monishdeb · 2018-05-21T07:19:55Z

Yes not only Joomla but all other CMSs tpl can share too, say for drupal.tpl it would be like

{if $config->debug}
{include file="CRM/common/debug.tpl"}
{/if}

<div id="crm-container" class="crm-container{if $urlIsPublic} crm-public{/if}" lang="{$config->lcMessages|truncate:2:"":true}" xml:lang="{$config->lcMessages|truncate:2:"":true}">

{include file="CRM/common/CMSPrint.tpl"}

</div>

but then it will still have few duplicate code which is the outer div structure and $config->debug chunk

eileenmcnaughton · 2018-05-21T07:23:07Z

@monishdeb that doesn't seem too bad in terms of duplication

monishdeb · 2018-05-21T20:02:16Z

Merging as per tag.

civicrm-builder added the master label Apr 5, 2018

eileenmcnaughton reviewed Apr 5, 2018

View reviewed changes

monishdeb mentioned this pull request Apr 5, 2018

(accessibility/misc-core-fixes/issues/3) Add accessible labels #11939

Closed

monishdeb force-pushed the access-3-1 branch from 27e27c5 to 4044d1b Compare April 5, 2018 09:29

eileenmcnaughton added the merge on pass label Apr 5, 2018

eileenmcnaughton removed the merge on pass label Apr 5, 2018

monishdeb force-pushed the access-3-1 branch 2 times, most recently from 7d23d48 to 80a3926 Compare April 5, 2018 11:27

monishdeb force-pushed the access-3-1 branch from 80a3926 to 1f188cd Compare April 6, 2018 13:25

monishdeb force-pushed the access-3-1 branch from 1f188cd to 339e079 Compare May 18, 2018 12:40

eileenmcnaughton reviewed May 20, 2018

View reviewed changes

(accessibility/misc-core-fixes/issues/3) Remove CMS specific print he…

8c902e9

…ader templates that share same code

monishdeb force-pushed the access-3-1 branch from 339e079 to 8c902e9 Compare May 21, 2018 06:51

monishdeb commented May 21, 2018

View reviewed changes

eileenmcnaughton added the merge on pass label May 21, 2018

monishdeb merged commit 9e1dde3 into civicrm:master May 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(accessibility/misc-core-fixes/issues/3) Remove CMS specific print header templates that share same code #11944

(accessibility/misc-core-fixes/issues/3) Remove CMS specific print header templates that share same code #11944

monishdeb commented Apr 5, 2018 •

edited

Loading

monishdeb commented Apr 5, 2018

eileenmcnaughton Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

seamuslee001 commented Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

monishdeb commented Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

monishdeb commented Apr 6, 2018

eileenmcnaughton commented Apr 6, 2018

JoeMurray commented May 18, 2018

eileenmcnaughton May 20, 2018

eileenmcnaughton May 20, 2018

eileenmcnaughton May 20, 2018

eileenmcnaughton commented May 20, 2018

monishdeb May 21, 2018

monishdeb commented May 21, 2018 •

edited

Loading

eileenmcnaughton commented May 21, 2018

monishdeb commented May 21, 2018

eileenmcnaughton commented May 21, 2018

monishdeb commented May 21, 2018

(accessibility/misc-core-fixes/issues/3) Remove CMS specific print header templates that share same code #11944

(accessibility/misc-core-fixes/issues/3) Remove CMS specific print header templates that share same code #11944

Conversation

monishdeb commented Apr 5, 2018 • edited Loading

Overview

monishdeb commented Apr 5, 2018

eileenmcnaughton Apr 5, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Apr 5, 2018

seamuslee001 commented Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

monishdeb commented Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

monishdeb commented Apr 6, 2018

eileenmcnaughton commented Apr 6, 2018

JoeMurray commented May 18, 2018

eileenmcnaughton May 20, 2018

Choose a reason for hiding this comment

eileenmcnaughton May 20, 2018

Choose a reason for hiding this comment

eileenmcnaughton May 20, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented May 20, 2018

monishdeb May 21, 2018

Choose a reason for hiding this comment

monishdeb commented May 21, 2018 • edited Loading

eileenmcnaughton commented May 21, 2018

monishdeb commented May 21, 2018

eileenmcnaughton commented May 21, 2018

monishdeb commented May 21, 2018

monishdeb commented Apr 5, 2018 •

edited

Loading

monishdeb commented May 21, 2018 •

edited

Loading