Skip to content

Conversation

skipjack
Copy link
Collaborator

Re-opening after accidentally closing #325.

@skipjack
Copy link
Collaborator Author

skipjack commented Nov 16, 2016

@bebraw @SpaceK33z fixed build errors so this should be good to go, feel free to review again.

@SpaceK33z
Copy link
Member

There is one thing that is bothering me a bit: if you click on Documentation -> Output, the submenu doesn't expand (showing the options in Output). I have to click on the small arrow icon next to it to expand it.

Note that I tested this with npm run build-test.

@skipjack
Copy link
Collaborator Author

skipjack commented Nov 16, 2016

Weird, that behavior doesn't work on npm run build-test but does work on the dev-server. I didn't change anything in the Sidebar, this PR was just for the top-level Navigation component (i.e. adding that little children section) and sorting/renaming of articles. I'll take a look though and see if I can figure out what's going on.

@skipjack
Copy link
Collaborator Author

It's a problem with the currentPage prop in Sidebar which is somehow not being computed properly on build.

@NejcZdovc
Copy link
Collaborator

Maybe this issue is related #308. I don't know why @bebraw closed it.

@bebraw
Copy link
Contributor

bebraw commented Nov 16, 2016

Ah, yeah. It's the same issue. Reopened.

@skipjack
Copy link
Collaborator Author

skipjack commented Nov 16, 2016

Yeah I tried to debug it a bit, with very little luck, the odd thing is the Navigation component is essentially passed the same thing from Site but that url works fine. We could always go back to using window but I think @bebraw was trying to avoid that (which I agree makes sense).

However, seeing as this issue is somewhat separate from this PR maybe I can fix the one conflict later today and then merge this? Any Sidebar issues could be tackled all at once when we implement the new design...

@bebraw
Copy link
Contributor

bebraw commented Nov 16, 2016

@skipjack Yeah, we can resolve it separately. Probably good to do a little session over it when we are both online at the same time.

@skipjack
Copy link
Collaborator Author

Sounds good, or if you and @NejcZdovc want to try to resolve it before the redesign PR that's cool too. Either way 👍

@SpaceK33z
Copy link
Member

So @skipjack, this is good to merge I think? After fixing the merge conflict you can merge it :).

@skipjack skipjack merged commit ba2a629 into develop Nov 17, 2016
@skipjack skipjack deleted the feature/structure-n-navigation branch November 17, 2016 05:50
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.

4 participants