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

[related #6080] Fix startOffset issue when useCookie is true #6902

Merged
merged 2 commits into from
May 7, 2016

Conversation

gunjanpatel
Copy link
Contributor

Thanks @Erftralle for reporting issue in #6080 (comment)

Testing and Reproduce info

Please follow the instructions given in #6080 to reproduce the issue.

You can set startOffset and enable useCookie by setting options shown as below.

$options = array(
    'startOffset' => 3, 
    'useCookie'   => true
);

// Using options
echo JHtml::_('tabs.start', 'pane', $options);

No Backward Compatibility issue

Please also check and confirm that it's not breaking backward compatibility.

@Erftralle
Copy link
Contributor

After applying the patch and using your sample code always Tab4 will be opened.

But what I would expect when using

$options = array(
    'startOffset' => 3, 
    'useCookie'   => true
);

is that Tab4 is shown on initial page load (localStorage or cookie not yet set) and then the last selected tab on all subsequent page loads.

@gunjanpatel
Copy link
Contributor Author

Thanks @Erftralle for testing.
Yes, is useCookie is false then tabs will not be remembered in browser local storage or cookie.

@Erftralle
Copy link
Contributor

Thanks @Erftralle for testing.

Unfortunately for me it was not a succesfull one :( .

Yes, is useCookie is false then tabs will not be remembered in browser local storage or cookie.

Yes, I know that. Is this answer related to my previous post? If yes, I don't understand you. I was talking about another test case and a different behaviour I would expect.

@gunjanpatel
Copy link
Contributor Author

Ahh ok. Then I think didn't understand your question clearly. Is another bug found?

As per my opinion Test cases would be like,

  • UseCookie => false and startOffset > 0 is given, tab should be selected which is given in startOffset.
  • UseCookie => false and startOffset = 0 (default), then first tab should be selected.
  • UseCookie => true and startOffset > 0 then given tab index in startOffset should be shown as selected (even if you change tab, it should ignore browser storage or cookie when start offset is given)
  • UseCookie => true and startOffset = 0 then it should follow the browser storage or cookie values, mean when you will select tab and refresh the page it will show the tab which was selected before refreshing the page.
    This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6902.

@Erftralle
Copy link
Contributor

Seems that we disagree about the expected behaviour of the third test case you mentioned (useCookie => true and startOffset > 0).

Lets see if there are other testers having an opinion to that point.

@slibbe
Copy link

slibbe commented Jul 11, 2015

I cannot reproduce, as tab3 is opened after the reload (after first selecting tab1 & tab2); no patch applied.

@josien
Copy link

josien commented Jul 11, 2015

Same here, I cannot reproduce the issue (tried in com_search).


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6902.

@gunjanpatel
Copy link
Contributor Author

@Erftralle @josien @slibbe can you guys try it again. There was an issue in default behaviour of using storage. By Default storage is not enabled.
Thanks for your tests.

@yvesh
Copy link
Member

yvesh commented Oct 24, 2015

Okay, we had a big discussion on this at PBF 2 in JUG Munich. It is working as expected, but are you sure on this?

Issues:

1.) IF you want to have the first (0) to be open this is not possible, offset is ignored when you use cookie and offset is zero.

2.) useCookie has no use if start offset is greater then zero.

From a developer perspective this is irritating!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6902.

@yvesh
Copy link
Member

yvesh commented Oct 24, 2015

I have tested this item ✅ successfully on 17541d4


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6902.

@degobbis
Copy link
Contributor

I have tested this item ✅ successfully on 17541d4

I see it like @yvesh


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6902.

@zero-24
Copy link
Contributor

zero-24 commented Oct 24, 2015

Needs Review so a Maintainer can take the decision on your points. Thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6902.

@Kubik-Rubik
Copy link
Member

Thank you @gunjanpatel! We decided to merge this PR.

@Kubik-Rubik Kubik-Rubik merged commit c3569e0 into joomla:staging May 7, 2016
@Kubik-Rubik Kubik-Rubik added this to the Joomla 3.6.0 milestone May 7, 2016
@gunjanpatel
Copy link
Contributor Author

Thanks you all for testing and merging.

@gunjanpatel gunjanpatel deleted the tabsCookieFix branch May 9, 2016 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants