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

Extract selectedChild tabheader functionality and enable for Manage Events #12747

Merged
merged 1 commit into from
Oct 10, 2018

Conversation

mattwire
Copy link
Contributor

Overview

Some pages with tabs in CiviCRM allow you to link directly to a specific tab using the URL parameter &selectedChild=tabname (eg. for the contact summary to link directly to the contributions tab tabname="contribute"

This is currently supported for:

  • Contact tabs
  • Contribution page tabs
  • Messagetemplates tabs
  • Extensions tabs
  • Profiles tabs

... But the code is duplicated in each template.

This PR extracts the duplicate code to a common shared template and adds support to the event page tabs.

Before

  • Duplicated code in template for each instance.
  • No support in event tabs.

After

  • Shared code which can be included anywhere tabs are implemented.
  • Support for selectedChild in event tabs.

Technical Details

For the events tab we have to retrieve the selectedChild parameter. Other than that this is just template extraction.

Comments

@civibot
Copy link

civibot bot commented Aug 29, 2018

(Standard links)

@mlutfy
Copy link
Member

mlutfy commented Aug 30, 2018

jenkins, test this please

@mattwire mattwire force-pushed the tabheader_extract_and_events branch 2 times, most recently from 034c5d6 to 8d536fa Compare September 5, 2018 13:06
{if $selectedChild}selectedTab = '{$selectedChild}';{/if}
{literal}
CRM.$(function($) {
var tabIndex = $('#tab_' + selectedTab).prevAll().length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a semicolon at the end of this line -for style even if it works without

CRM.$(function($) {
var tabIndex = $('#tab_' + selectedTab).prevAll().length;
$("#mainTabContainer").tabs({active: tabIndex});
$(".crm-tab-button").addClass("ui-corner-bottom");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class addition is lost in this change (intentionally or otherwise) - however, I think the selectedChild doesn't actually work on extensions with or without this patch -it does with this

diff --git a/CRM/Admin/Page/Extensions.php b/CRM/Admin/Page/Extensions.php
index a2d5e9cbad..eb556d4afb 100644
--- a/CRM/Admin/Page/Extensions.php
+++ b/CRM/Admin/Page/Extensions.php
@@ -56,6 +56,7 @@ class CRM_Admin_Page_Extensions extends CRM_Core_Page_Basic {

 $destination = urlencode($destination);
 $this->assign('destination', $destination);
  • $this->assign('selectedChild', CRM_Utils_Request::retrieve('selectedChild', 'Alphanumeric', $this));
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class loss also affects ContributionPage

@eileenmcnaughton
Copy link
Contributor

Testing notes (still wip)

Tested urls

work (before & after)
civicrm/admin/uf/group?reset=1&selectedChild=reserved-profiles
civicrm/admin/uf/group?reset=1&selectedChild=user-profiles

doesn't work (before & after)
civicrm/admin/extensions?reset=1&selectedChild=addnew

@mattwire what do you think about putting this line

$this->assign('selectedChild', CRM_Utils_Request::retrieve('selectedChild', 'Alphanumeric', $this));

into CRM_Core_Basic_Page::run()

it feels like a common enough & cheap enough thing it might make sense

{* Tab management *}
<script type="text/javascript">
var selectedTab = 'contributions';
{include file="CRM/common/TabSelected.tpl" defaultTab="contributions" tabContainer="#secondaryTabContainer"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure out how to select the recurring subtab (before or after this change)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton No, you can't :-( But maybe once this abstraction is in place we might be able to look at it

@mattwire mattwire force-pushed the tabheader_extract_and_events branch from 8d536fa to c1bd837 Compare October 10, 2018 08:31
@mattwire
Copy link
Contributor Author

Ok, I've addressed comments, extensions is now working. MessageTemplates uses CRM_Core_Page instead of CRM_Core_Page_Basic so I've not abstracted out the assigns there as I nearly fell into the rabbit hole again.

@eileenmcnaughton
Copy link
Contributor

thanks @mattwire - good to merge

@seamuslee001
Copy link
Contributor

Test fails unrelated, merging as per the tag

@seamuslee001 seamuslee001 merged commit b6462ff into civicrm:master Oct 10, 2018
@mattwire mattwire deleted the tabheader_extract_and_events branch October 10, 2018 22:20
@@ -107,3 +107,4 @@ CRM.$(function($) {
</script>
{/literal}
{include file="CRM/Event/Form/ManageEvent/ConfirmRepeatMode.tpl" entityID=$id entityTable="civicrm_event"}
{include file="CRM/common/TabSelected.tpl" defaultTab="settings"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire @eileenmcnaughton Currently this line breaks the manage event URLs if selectedChild is not appended to it. Eg

https://dmaster.demo.civicrm.org/civicrm/event/manage/fee?reset=1&action=update&id=3 goes to the settings page instead of opening the fee tab. Similarly all the configuration links from the manage event page goes to the settings page.

Maybe, we should just

  • remove this line from the file or
  • append selectedChild to the links in Manage Event Page. But we already specify the tab name in the link before the query string eg civicrm/event/manage/fee?reset=1.
  • Format the links to look like civicrm/event/manage?..selectedChild=fee (might need xml/upgrade changes along with fixing the links in core).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire @jitendrapurohit maybe we should assign defaultTab from php?

@mattwire I think this needs to be fixed in the rc

Copy link
Contributor Author

@mattwire mattwire Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So before this PR the selectedChild stuff didn't work for events at all. Events is the only one that specifies the tab as part of the link.

  1. remove this line from the file

Specifying the "defaultTab" is standard across all usages of the tab selector code and is meant to be a fallback default if selectedChild is not defined.

  1. append selectedChild to the links in Manage Event Page. But we already specify the tab name in the link before the query string eg civicrm/event/manage/fee?reset=1.

This is probably the easiest / safest way to resolve this, unless we assign defaultTab from PHP in 1.

  1. Format the links to look like civicrm/event/manage?..selectedChild=fee (might need xml/upgrade changes along with fixing the links in core).

This would be more standard and is what we should probably aspire to for consistency, but is also much more likely to cause breakage.

@mattwire
Copy link
Contributor Author

@jitendrapurohit @eileenmcnaughton Please see Links to manage event tabs broken following work on "selected tab" functionality.

@colemanw
Copy link
Member

I think the work in this PR arose from an unfortunate lack of communication. Back in 2013 I worked to get the "default tab" functionality out of the .tpl and into a proper javascript file: templates/CRM/common/TabHeader.js. Whatever leftover code was in the template didn't need to be kept, it could just be removed in favor of the pure js approach.

See https://lab.civicrm.org/dev/core/-/issues/2215#note_50975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants