Add layout properties to the UI store and cookie to store them #1955
Conversation
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1955 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
|
Size Change: -56 B (0%) Total Size: 823 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
add/ui_state_cookie
branch
2 times, most recently
from
7037c00 to
e832116
Compare
obulat
force-pushed
the
add/ui_state_cookie
branch
from
e832116 to
5b57ece
Compare
obulat
changed the title
obulat
added
🟧 priority: high
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was updated 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes |
There was a problem hiding this comment.
@obulat Were you thinking of refactoring useCookies? I think the API could be improved if we used the approach I suggested elsewhere. Rather than passing the name parameter to useCookies, we could get and set the cookie by name in the closure functions the composable produces. It would also remove the need to have to pass an explicit generic parameter to useCookies like we'll need to now to get the right types for the closures.
Additionally, I am wondering if useCookies needs to expose a reactive object for the cookies in case the values change. For example, consider if we are relying on the value of the isDesktopLayout value rather than computed breakpoints. If we adjust the width of the viewport then will isDesktopLayout be recomputed (to false, if we go below a desktop layout width) and then cause a re-render? Right now I think the value might be static, even with the Pinia store, because the store values aren't hooked in (as far as I can tell) to a media query watcher.
Yes, I will refactor based on your suggestion, @sarayourfriend. As for the second question, I've added the 'reactive object for the cookies' sounds really interesting, and I will see if that can be used during the refactoring. |
obulat
force-pushed
the
add/ui_state_cookie
branch
2 times, most recently
from
d1cc631 to
9b599fa
Compare
| @@ -16,7 +20,7 @@ interface Options { | |||
| * Reactive Media Query. | |||
| */ | |||
| export function useMediaQuery( | |||
| query: string, | |||
| query: MaybeComputedRef<string>, | |||
There was a problem hiding this comment.
To update the desktop threshold based on the new_header flag status, we need to pass a computed value here.
src/middleware/middleware.ts
Outdated
| /* UI store */ | ||
|
|
||
| const uiStore = useUiStore($pinia) | ||
| const isMobileUa = app.$ua ? app.$ua.isMobile : false |
There was a problem hiding this comment.
It is expected that isMobileUa can be set once per request, and we do not need to change its value or update a cookie after that.
src/stores/ui.ts
Outdated
| * and `md` with the `old_header`). | ||
| * @param setCookieFn - sets the app cookie. | ||
| */ | ||
| updateBreakpoint(isDesktopLayout: boolean, setCookieFn: CookieSetter) { |
There was a problem hiding this comment.
We could add a check for isMobileUa here and only set the isDesktopLayout to true if isMobileUa is false. However, I'm not sure if it makes sense to use a mobile layout for lg mobile screens.
There was a problem hiding this comment.
I think you're right, that it doesn't make sense. To me, it would make sense to rely exclusively on the breakpoint information if we have it and fall back to UA sniffing only when we don't yet know any breakpoint information from the cookie.
src/stores/ui.ts
Outdated
| * and `md` with the `old_header`). | ||
| * @param setCookieFn - sets the app cookie. | ||
| */ | ||
| updateBreakpoint(isDesktopLayout: boolean, setCookieFn: CookieSetter) { |
There was a problem hiding this comment.
I think you're right, that it doesn't make sense. To me, it would make sense to rely exclusively on the breakpoint information if we have it and fall back to UA sniffing only when we don't yet know any breakpoint information from the cookie.
obulat
force-pushed
the
add/ui_state_cookie
branch
from
a56d375 to
e1c1860
Compare
src/composables/use-cookies.ts
Outdated
| secure: isProd, | ||
| } | ||
|
|
||
| export const useCookies = (app: NuxtAppOptions) => { |
There was a problem hiding this comment.
Naïve question: why not use the useCookies composable from VueUse?
There was a problem hiding this comment.
Thank you fro asking this question, @dhruvkb !
I've started answering why we can't use it, and then found a much nicer solution.
We use the cookie-universal-nuxt which injects $cookies to the Nuxt app. This means that we can only set cookies in components and not in a Pinia store because you can't access app from outside of a setup function.
That said, researching this issue, I've found that $nuxt was added to the Pinia Nuxt plugin, so we can set the cookies from the store by calling this.$nuxt.app.$cookies.set().
There was a problem hiding this comment.
Changes look great to me, always nice to cut out some extra code (though I have to admit I found the composable nice to work with, especially w/r/t to typing).
To get cookies-universal-nuxt's types working, can you please add cookies-universal-nuxt to the types array in tsconfig.json? Otherwise $cookies is typed as any 😱
Create use-cookies composable Move all the UI cookie updates to the UI store Add cookieOptions
obulat
force-pushed
the
add/ui_state_cookie
branch
from
dd5279c to
630d501
Compare
Fixes
Fixes #974 by @sarayourfriend
Description
This PR adds the following layout properties to the
uipinia store:isDesktopLayout: whether the site should use thedesktopormobilelayout (i.e., different headers, displaying menus in a modal on mobile or in a popover/sidebar on desktop).isMobileUa: whether the User Agent is mobile. This is currently not used for layout, and I'm not sure whether we really need it or not. One thing I suspect this will be necessary for is any mobile-specific handling of the audio or video players.innerFilterVisible: to keep track of the filters state. The components should useisFilterVisibleas that getter updates theisFilterDismissedif the screen width has changed and we need to update the cookie state accordingly.isFilterDismissed: on a desktop layout, the filter sidebar is always open by default. However, if the user dismisses it, we want to save this preference across requests.The values are initialized from a cookie (and the request properties) in the
middleware. Then, on every change of the layout, user agent or filterdismissedvalue, we save the value to the state and to the cookie.Additional details
Unlike the approach outlined in the linked issue, this PR does not save the exact breakpoint. Instead, it saves whether the layout should be
desktopormobile(based on the current threshold:mdforold_headerandlgfornew_header). This way, we can reuse theisMinScreencomposable that usesmatchQueryfor this instead of adding a separateresizelistener. Besides, we were going to use the current breakpoint to compute whether we should use a desktop or mobile layout anyways.This PR adds
two composablesone composable:to save data in cookies. It uses the NuxtAppThis turned out not necessary because Pinia Nuxt plugin can accessappas a parameter to enable setting the cookies from outside of thesetupfunction, namely, from theuistore.appusingthis.$nuxt.app.default.vueandblank.vuelayouts.Testing Instructions
I've added the unit tests for the
uistore. The changes to the store will also be tested in #1956 which uses theuistore to set the layout properties.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin