Fixes issue #2425: Set navbar buttons to the active state #4411
Conversation
|
+1 if this means I don't have to add ui-btn-active ui-state-persist manually on the respective page navbar anymore |
|
@frequent - You would still have to add class ui-btn-active if you want to set the current page to the active state in the navbar, but you wouldn't have to add ui-state-persist anymore. |
|
+0.5 then :-D |
|
@frequent Haha... If you can figure out a way to have the framework recognize what navbar button is refering to the current page, no matter how the link on the anchor is defined, I will +1.5 on your pull request ;-) |
|
Hi @uGoMobi
The ui-btn-active is lost on the footer then. |
|
@MauriceG Hi Maurice! Thanks for testing this. Actually, the bug that you described also occurs without the patch. It is caused by something else than these changes. Will look into it. Jasper |
|
@uGoMobi Hi Jasper! |
|
Some questions: This change seems convenient for the use case described, but are we sure that this is the expected behavior for everyone who adds an active class to the original state of a button on a page? Also, if this is the default behavior for adding ui-btn-active now, how can people opt-out of this to get the original behavior (in which the class is only used for init)? Are there other issues at play here aside from the inconvenience of adding that persist class, and that it apparently should be documented more broadly than for persistent toolbars alone? |
|
Thanks for your feedback @scottjehl "...but are we sure that this is the expected behavior for everyone who adds an active class to the original state of a button on a page?" - The change only affects buttons in navbars. "...how can people opt-out of this to get the original behavior (in which the class is only used for init)?" - There is no opt-out because I can't think of any possible use case. It would mean random behaviour: when a page remains in the DOM the active state will only be visible the first time the page is shown, but when a page is removed from the DOM you will see the active state again when you go back to that page. "Are there other issues at play..." - No. The alternative is to explain at the "Navbars" page in the docs that you have to add class ui-state-persist as well. |
|
"The change only affects buttons in nav bars." Sorry I should have been clearer about the question. I guess it's just hard to be sure of the reasons one might add an active state to a navbar in a page, but I think one use case could be if they scripted client-side tabs into a page. In that case, the initial state is only there to represent the active tab, and if the tab is changed, the active state should swap- and persist - to match the new active tab. FWIW, I'm very much in agreement that the docs should be clearer about the availability in this class for navbars - not just those in persistent toolbars. But beyond that, I'm unsure how aggressive we should get with making the theme class itself sticky. On May 21, 2012, at 10:41 AM, Jasper de Groot wrote:
|
|
Ok, just re-read through this whole thread and although I agree that having the active state be persistent after reload would be convenient, we need to be very careful about introducing changes that could affect all the code out there in the world. Because we're following semantic versioning, anytime we change the default behavior, appearance or API of a widget it could be a problem. If we changed the default to be persistent, then we'd need to add another class to set these back to the old behavior anyway so we'd be adding complexity and unexpected changes for the sake of convenience (even though I agree this would have been a good default). In this case, since we already have a mechanism to make the active state persistent, I think we should handle this by improving the documentation to make it clear this applies to all navbars, not just those in fixed or persistent footers. That will be a big help. @uGoMobi - mind closing and making changes to the docs. I think adding detailed info and even demos to illustrate this is a fantastic idea. We should check the data-attr reference too. |
|
@toddparker @scottjehl - I agree, now I am aware of other possible use cases, that it's not a good idea to set this as default. I will take care of making the changes to the docs. @MauriceG - Do you mind creating a new issue for that bug you found? |
Issue:
In the docs on the "Navbars" page is explained that you can add class ui-btn-active in the markup to give a navbar button the active state. Only at the "Persistent toolbars" page is mentioned that you need to add class ui-state-persist as well.
In fact you always need to add class ui-state-persist to make sure the button get the active state again on pageshow. Otherwise it only gets the active state again when the navbar is enhanced again, which does not happen if that page remains in the DOM.
Fix:
I added a function to the navbar widget that adds class ui-state-persist to anchor elements with class ui-btn-active during enhancement.
Additionally I changed the function that restores class ui-btn-active so it first removes this class from all navbar links. This is for the examples on the "Navbars" page which are not real links, so removing ui-btn-active upon succesful navigation does not happen.
In the docs I removed the info on the "Persistent toolbars" page about adding class ui-state-persist manually and I removed this class from the markup.
Fixes issue #2425