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 'selectedChild' parameter for pages with tabs #17066

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

mattwire
Copy link
Contributor

Overview

This makes sure we always load the required javascript to allow selecting a tab via the URL parameter selectedChild. This was not working with events and some of the others but was working with the contact summary tabs.

Before

Tab selected via selectedChild URL parameter not working on various pages.

After

Tab selected via selectedChild URL parameter working on various pages.

Technical Details

In order to work, the javascript in TabSelected.tpl must be loaded. But in many cases it was not. It is now loaded whenever TabHeader.tpl is loaded - it may make sense to combine the two template files.

Comments

Testing

To test, navigate to each of the pages which have tabs and try the following URLs and check expected results. (modify the first part to match the test system)

Profiles:
Manage event:
Contact summary:
MessageTemplates:
Extensions:

@civibot
Copy link

civibot bot commented Apr 13, 2020

(Standard links)

@civibot civibot bot added the master label Apr 13, 2020
@@ -37,3 +37,4 @@
{/if}
<div class="clear"></div>
</div> {* crm-content-block ends here *}
{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.

Haven't we lost some nuance on defaultTab here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, but in practise they all default to the first tab so the behaviour before/after is the same. Also to add some fun selectedChild can and normally is set on the PHP side so the default is taken from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a Smarty expert, but could we just pass defaultTab down through TabHeader.tpl to TabSelected.tpl?
But agree with Matt, the default is duplication/overkill if the selectedChild is always set and the default would always be the first tab anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

This currently breaks the default loading of tabs when contribution page is accessed from the action links. Eg from https://dmaster.demo.civicrm.org/civicrm/admin/contribute?reset=1 -> select Configure > Membership Settings should navigate to the Membership tab directly but instead is navigated to the first tab of the page(Settings).

Can be replicated by simply clicking on https://dmaster.demo.civicrm.org/civicrm/admin/contribute/membership?reset=1&action=update&id=1 (opens settings page instead of membership). Do we aim to support selectedChild parameter in this URL?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jitendrapurohit did you open a gitlab?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire imo - maybe we should just revert this line for the fix?

@artfulrobot
Copy link
Contributor

Note: CRM/Contribute/Page/Tab.tpl includes

{include file="CRM/common/TabSelected.tpl" defaultTab="contributions" tabContainer="#secondaryTabContainer"}

which is the only thing stopping us moving TabSelected.tpl inside TabHeader.tpl.

@artfulrobot
Copy link
Contributor

(CiviCRM Review Template WORD-1.2)

@eileenmcnaughton
Copy link
Contributor

Thanks for the review @artfulrobot - this is a code area @mattwire did some earlier improvements on (maybe 2 years back). The defaultTab being set in tabHeader still seems a bit weird - but based on the review & the triviality I'm happy to leave it being a bit weird

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.

4 participants