-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fixed navbar Mobile Collapse not giving correct example #1813
Conversation
…h what the JavaScript SideNav says.
I'll just reference #1746 |
In regards to that PR you made, I'm actually going to make all those changes for the icons... I'm closing this PR, in favor is a newer one. |
I honestly wouldn't bother, because they will almost certainly give you the same reply they did me, and the PR will end up sitting for weeks, or just outright being rejected. Merging (no pun intended) all of the changes is rather useless, as others have tried to get them pulled and have been rejected. In addition, the dev team here has outright refused to remove (and replace) dead demo links #1791 et al. |
Fair enough. |
The If there is any old documentation from the jade folder that is still using the old |
Those new icons aren't working indeed. If what you're saying is true, then the documentation is ahead of the implementation. (Which is just as bad as the other way around) |
@carlosperate As I pointed out in #1746 (and several others, this issue included,) the new implementation is broken. There is no excuse for #1791 et al. though. |
I'm not debating that there might be issues with the new icons, just that if anything is broken then it can be looked into and fixed, rather than revert back to the old implementation in the documentation. About the specific icon issue you mentioned in your last comment from #1746, has 43d43c2 fixed that one? If not then we can open a new issue to capture that problem. |
I'm updating the docs atm and I'll make a new PR for it. |
@carlosperate See #1814 |
And about the buttons not working, please see this commit: https://github.com/TheSerenin/periodic-table/commit/3cf19df9df8fad62e0217cf936649f9358dea3a3 |
The example on the NavBar page, at Mobile Collapse, doesn't give the right example. It's saying you need to add
But it should be
Like how it is on the SideNav page under JavaScript.
This commit fixed that.