inland-empire-software-development / spark

Open source learning management system.
MIT License
16 stars 6 forks source link

Sidebar active item #109

Closed JacobSNGoodwin closed 4 years ago

JacobSNGoodwin commented 4 years ago

@saynegrojas, could you take a look at the sidebar updates?

I made the parent-level item stay highlighted and the children items stay expanded on navigation.

There are a few other problems I am considering fixing. Do you have an opinion?

  1. I can't stand the scroll bar overlay on the border, though this may not be fixable as it's very browser dependent. See image below. Screen Shot 2020-03-02 at 2 44 26 PM
  2. I would like to default to closing the sidebar when navigating on mobile. What do you think?
saynegrojas commented 4 years ago

Hey Jacob, I think it'll be much easier for users this way rather than having to keep pressing the parent button all the time. As far as the problems you had, I agree with you on both counts. The scroll bar doesn't look that bad, but, you're right, it'll look better without it. Though, if it is a browser problem, we might have to just keep it for now. Also, I think that's a good idea to close the sidebar on default when on mobile, makes it cleaner. But let's get more opinion and see what everyone thinks. Good job man.

lloan commented 4 years ago

I agree on both items too @JacobSNGoodwin. The sidebar's scrollbar is annoying, but if its a necessary evil, then so be it. If we can get away with using the global scrollbar, that'd be great. I know that's what WordPress does - but this sidebar already feels better than WordPress's sidebar so great job man in all other aspects.

JacobSNGoodwin commented 4 years ago

@lloan,

I have it working better now. The only exception if that Firefox, when I set it to responsive mode only, still pushes the navbar into the sidebar container with the thin (I actually like this), slightly transparent navbar. Otherwise it looks better now on both Chrome and Safari. Since Firefox mobile is probably like .5% of users, it's probably ok.

I just pushed this minutes ago.

I now just have some sort of bug in the state of open menus I'm setting in context. Sometimes one is collapsing on navigation. Trying to figure that out.

Also, in an ideal world, I'd keep scroll position across page rerenders, but I might have to create a piece of state for that, too. 🤦‍♂

lloan commented 4 years ago

Saving the scroll state would be sick @JacobSNGoodwin - that's actually one of the things that most people consuming content usually ask for on things like blog sites. At work, that's something on our backlog, lol. But it would definitely require quite a bit of thought. Great job on this component, its become a lot more like its own small app within an app, but still modular.

JacobSNGoodwin commented 4 years ago

Hey folks, I fixed the above issues, except in firefox for mobile devices. The navbar always covers up.

The only thing I don't like if the scrollTop position of the sidebar isn't persistent. I tried a number of technique to store the scrollTop on the context. The context was setting the proper value, but no matter how I tried to set it when a new page rendered, it's jump to a different position.

If someone has a good solution, I'm glad to implement it, but I feel like this is good enough for now.

From some tweets and github issues, nextjs should implement better support for persistent layouts beyond what is supported by _app.tsx. So this would allow the sidebar to keep its state without using context.