Problem/Motivation

There are some cases where I don't want quicktabs to remember the last clicked tab on page reload.

Proposed resolution

Add a configuration checkbox on the tab settings page to remember the last clicked tab per quicktabs instance.

CommentFileSizeAuthor
#27 quicktabs--3189439-MR37.patch7.28 KBloze
#19 after_patch.png96.1 KBindrapatil
#18 3189439-after-patch.png265.32 KBsahil.goyal
#18 3189439-before-patch.png307.25 KBsahil.goyal
#14 interdiff-7-13.txt1004 bytesfloydm
#14 quicktabs-remember-tab-setting-3189439-13.patch7.79 KBfloydm
#9 quicktabs-remember-tab-setting-3189439-7.patch7.19 KBloze
#3 quicktabs-remember-tab-setting-3189439-3.patch6.39 KBloze
#2 quicktabs-remember-tab-setting-3189439-2.patch6.39 KBloze

Issue fork quicktabs-3189439

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

loze created an issue. See original summary.

loze’s picture

loze commented
Status: Active » Needs review
StatusFileSize
new quicktabs-remember-tab-setting-3189439-2.patch6.39 KB

Here is a patch that provides a setting on the quicktabs instance config page for "Remember list clicked tab" which determines if the cookie is used in the js.

thanks.

loze’s picture

loze commented
StatusFileSize
new quicktabs-remember-tab-setting-3189439-3.patch6.39 KB

Just correcting spelling mistakes in the last patch.

loze’s picture

loze commented
Related issues: +#3177587: Better default tab behavior

I forgot that I had submitted a similar patch a while back. #3177587: Better default tab behavior

So I've closed that issue in favor of this one which I think is a better.

lubwn’s picture

lubwn commented

Patch from #3 works like a charm! Thank you a lot.

I suggest we push this to production, applied cleanly and works nicely.

loze’s picture

loze commented
Status: Needs review » Reviewed & tested by the community
loze’s picture

loze commented
Status: Reviewed & tested by the community » Needs review

This is broken now that we are using the core/js-cookie library instead of jquery cookie. Will provide a new patch shortly

loze’s picture

loze commented
Status: Needs review » Needs work
loze’s picture

loze commented
Related issues: +#3298649: Replace jquery.cookie with js-cookie
StatusFileSize
new quicktabs-remember-tab-setting-3189439-7.patch7.19 KB

This patch gets the cookies working with js-cookie. Thanks.

loze’s picture

loze commented
Status: Needs work » Needs review
niklp’s picture

niklp commented

Would I be correct in saying that this is the part of the patch that fixes the "$.cookie is not a function" error?

-(function ($, Drupal, drupalSettings, once) {
+(function ($, Drupal, drupalSettings, once, cookies) {
loze’s picture

loze commented

its part of it but not all of it.
see: https://www.drupal.org/node/3104677

floydm’s picture

floydm commented

The config change and javascript from comment 9 work great but AFAICT the QuickTabsInstanceEditForm does not save or reflect the remember_last_clicked_tab value from the config.

The attached patch makes explicit the saving of that value.

floydm’s picture

floydm commented
StatusFileSize
new quicktabs-remember-tab-setting-3189439-13.patch7.79 KB
new interdiff-7-13.txt1004 bytes

keshavv made their first commit to this issue’s fork.

keshavv opened merge request !14

keshavv’s picture

keshavv commented

The patch given in #9 and #14 failed to apply due to some recent code changes.
The patch #9 also combines the Cookie issue, Which is already fixed on latest 8.x-3.x Branch.
So I have created the MR that will only add the configuration to toggle the remember last clicked tab.
Please review the MR
Thank you.

sahil.goyal’s picture

sahil.goyal commented
StatusFileSize
new 3189439-before-patch.png307.25 KB
new 3189439-after-patch.png265.32 KB

Thanks @keshavv as described #9 and #14 are now getting failed and some changes already been committed.
MR!14 is working correctly and tab history should be configurable and cookie can now and confirming its working fine and no any issue has been observed, attaching screenshots for the references.
RTBC+1

indrapatil’s picture

indrapatil commented
StatusFileSize
new after_patch.png96.1 KB

Hi
I have tested the patch it working I attached the screenshot for reference.

keshavv’s picture

keshavv commented
Status: Needs review » Reviewed & tested by the community
kburakozdemir’s picture

kburakozdemir commented

patch#14 works for me. thanks. RTBC+1...

smustgrave’s picture

smustgrave commented
Version: 8.x-3.x-dev » 4.0.x-dev
Status: Reviewed & tested by the community » Needs work

Will need an upgrade hook + test coverage

loze opened merge request !37

loze’s picture

loze commented

MR 37 is against 4.0.x
Still needs update hook and test coverage.

loze changed the visibility of the branch 3189439-last-clicked-tab to hidden.

loze changed the visibility of the branch 3189439-last-clicked-tab to active.

loze’s picture

loze commented
StatusFileSize
new quicktabs--3189439-MR37.patch7.28 KB

Ignore MR14, that is no longer valid and I am unable to close it.

MR37 is the latest, against 4.0.x

Here is a patch for composer that should work with the 4.0.x dev branch

loze’s picture

loze commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

There’s no upgrade hook or test coverage

loze’s picture

loze commented

Ive added an update hook which sets the new default value on all existing QT instances.

Added the config variable to QuicktabsConfigSchemaTest but I am unsure how to go about testing the actual functionality and could use some help, thanks.

smustgrave’s picture

smustgrave commented

So the update hook will need to be in a post update hook and use a ConfigImporter of some kind to do in batches.

loze’s picture

loze commented
Status: Needs work » Needs review

Ive added quicktabs.post_update.php. Is this more along the line of what you are asking for?