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 dev/core#2215 & remove the tab selection inline script from TabHeader.tpl #19066

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Nov 28, 2020

Overview

Removes some redundant code that was selecting the default tab twice. https://lab.civicrm.org/dev/core/-/issues/2215

Before

Default tab set by tabHeader.js and then set again (sometimes incorrectly) by TabSelected.tpl.
In the case of Campaigns/Surveys, it was incorrect.

After

Default tab set just once.

Technical Details

This mostly gets the logic for selecting default tab out of the tpl. The final step would be to delete CRM/common/TabSelected.tpl and all references to it and the variables it uses.

@civibot
Copy link

civibot bot commented Nov 28, 2020

(Standard links)

@civibot civibot bot added the master label Nov 28, 2020
@seamuslee001
Copy link
Contributor

I did an r-run of this and confirmed that it works for Contribution Pages and fixes the Campaign Dashboard links. @colemanw does this need to be against the RC or is this regression old enough it isn't 'recent'?

@colemanw
Copy link
Member Author

I think this regressed about 6 months ago due to #17066
Probably doesn't need to be in the RC. It would be good to make sure this still works with Events as that was the intended fix in #17066.

@mattwire
Copy link
Contributor

@colemanw This seems to break messagetemplates admin UI:
image

@colemanw
Copy link
Member Author

colemanw commented Nov 28, 2020

@mattwire - indeed, that screen was relying on a messy artifact of TabSelected.tpl - in setting the default tab it was actually initializing the tabs!
Update: woa, that page actually had the <div id="mainTabContainer"> element twice! Once in the template and then including TabHeader.tpl added it a second time. It's a wonder it functioned at all.

@seamuslee001
Copy link
Contributor

@colemanw test fail looks related CRM_Contribute_Form_ContributionTest::testOpeningWidgetAdminPage

This is part of ongoing cleanup to get the logic for selecting default tab
out of the tpl, which was redundant with the logic already in tabHeader.js.
@eileenmcnaughton
Copy link
Contributor

@mattwire @seamuslee001 have the issues been addressed now?

@mattwire
Copy link
Contributor

mattwire commented Dec 4, 2020

Yes, thanks @colemanw

@mattwire mattwire merged commit 465fc97 into civicrm:master Dec 4, 2020
@eileenmcnaughton eileenmcnaughton deleted the tabCleanup branch December 4, 2020 10:42
@demeritcowboy
Copy link
Contributor

The removal of {include file="CRM/common/TabSelected.tpl" defaultTab="settings"} from templates/CRM/common/TabHeader.tpl makes the tabs on the extension admin screen stop being tabs (https://lab.civicrm.org/dev/core/-/issues/2233). Since putting it back sounds undesirable, any thoughts?

@mattwire
Copy link
Contributor

mattwire commented Dec 5, 2020

@demeritcowboy I knew there was somewhere else that broke when stuff changed! I think @colemanw is on top of how this is meant to work and we probably need a small PR to fix the extensions admin screen?

@mattwire
Copy link
Contributor

mattwire commented Dec 5, 2020

@colemanw @demeritcowboy See #19130

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

Successfully merging this pull request may close these issues.

5 participants