Persistent menuActive state with Local Storage #3011

davidangel · 2023-01-26T04:26:58Z

davidangel commented

2023-01-26 04:26:58 +00:00

I sometimes do a full page refresh and would prefer the menu state survive that.

frederick commented

2023-01-26 04:31:45 +00:00

Hi davidangel!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3011-persistent-menu-state--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi davidangel! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3011-persistent-menu-state--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen requested review from dpschen 2023-01-26 09:21:51 +00:00

dpschen added the

kind/feature

label 2023-01-26 09:21:57 +00:00

dpschen self-assigned this 2023-01-26 09:22:03 +00:00

dpschen commented

2023-01-26 10:16:01 +00:00

Thanks for the PR. This is a good point, I like this idea very much! 👍
Stumbled upon this already on my own.

One detail where I think the user would benefit if we would to make some small change is if for this case:

(desktop view) sidebar ist open
making window smaller => sidebar will automatically collapse
making window larger again => sidebar stays collapsed

This worked on the current version and 'broke' with your change.

What do you think of the following:

Instead of saving the state if the sidebar is open or not for all window widths we only save it for desktop widths (over 770px).

I think most people don't want the sidebar to remember the open state on mobile. If they want that we could introduce a second stored value only for that.

The onMounted(() => resize()) should be removed.

We replace our useEventListener for resize events with useWindowSize from vueuse. Now we only watch for changes of the returned width ref.

We should consider two cases:

We collapse the sidebar if the old value was larger than our breakpoint width aka if the window gets smaller than the breakpoint. But only at that exact moment.
If the window was smaller than the breakpoint but gets larger (comparing old and new value in a watcher again we read out the value saved in your introduced const menuActive = useStorage('menuActive', true) which will fall back to true.

I hope that makes sense. Maybe I also oversaw something. Also there should be other / better ways to implement this. I just wanted to give some idea.

Thanks for the PR. This is a good point, I like this idea very much! 👍 Stumbled upon this already on my own. One detail where I think the user would benefit if we would to make some small change is if for this case: 1) (desktop view) sidebar ist open 2) making window smaller => sidebar will automatically collapse 3) making window larger again => sidebar stays collapsed This worked on the current version and 'broke' with your change. ### What do you think of the following: Instead of saving the state if the sidebar is open or not for all window widths we only save it for desktop widths (over 770px). I think most people don't want the sidebar to remember the open state on mobile. If they want that we could introduce a second stored value only for that. The `onMounted(() => resize())` should be removed. We replace our `useEventListener` for `resize` events with [`useWindowSize` from vueuse](https://vueuse.org/core/useWindowSize/). Now we only watch for changes of the returned width ref. We should consider two cases: - We collapse the sidebar if the old value was larger than our breakpoint width aka if the window gets smaller than the breakpoint. But only at that exact moment. - If the window was smaller than the breakpoint but gets larger (comparing old and new value in a watcher again we read out the value saved in your introduced `const menuActive = useStorage('menuActive', true)` which will fall back to `true`. ----------- I hope that makes sense. Maybe I also oversaw something. Also there should be other / better ways to implement this. I just wanted to give some idea.

👍 1

davidangel commented

2023-02-02 03:33:58 +00:00

@dpschen – Thanks for the feedback! I've taken a stab at an approach approximating what you've suggested. Let me know what you think.

davidangel force-pushed persistent-menu-state from 91222c156c to 0ad7924878

2023-02-03 01:36:14 +00:00

Compare

konrad requested changes 2023-02-03 10:32:21 +00:00

konrad left a comment

Please remove the yarn.lock file. We're using pnpm to manage dependencies.

Please remove the `yarn.lock` file. We're using pnpm to manage dependencies.

davidangel requested review from konrad 2023-02-04 01:35:48 +00:00

dpschen commented

2023-02-08 12:03:16 +00:00

Hey @davidangel,

I rebased the branch and abstracted the functionality to a composable.
By using the composable useMediaQuery I could simplify the logic a bit, because we are able to watch that as a trigger to set the prefered setting.

See here:

https://kolaente.dev/vikunja/frontend/compare/main...dpschen/frontend:persistent-menu-state

What do you think?

Hey @davidangel, I rebased the branch and abstracted the functionality to a composable. By using the composable `useMediaQuery` I could simplify the logic a bit, because we are able to watch that as a trigger to set the prefered setting. See here: https://kolaente.dev/vikunja/frontend/compare/main...dpschen/frontend:persistent-menu-state What do you think?

👍 1

davidangel commented

2023-02-08 13:09:12 +00:00

@dpschen - I think this makes sense. Shipit?

🎉 1

konrad was assigned by dpschen

2023-02-08 14:26:21 +00:00

dpschen commented

2023-02-08 14:26:39 +00:00

@konrad anything to add?

dpschen approved these changes 2023-02-08 14:45:20 +00:00

konrad commented