This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Use the UI store properties to set the layout properties #1956
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1956 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: -3.23 kB (0%) Total Size: 811 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
use_cookie_for_layout
branch
2 times, most recently
from
fe40051 to
29c7cd8
Compare
obulat
commented
Nov 4, 2022
obulat
commented
Nov 4, 2022
obulat
force-pushed
the
use_cookie_for_layout
branch
from
29c7cd8 to
54c1f5e
Compare
obulat
force-pushed
the
add/ui_state_cookie
branch
from
7037c00 to
e832116
Compare
7 tasks
obulat
force-pushed
the
use_cookie_for_layout
branch
from
54c1f5e to
e94b1da
Compare
obulat
force-pushed
the
add/ui_state_cookie
branch
from
e832116 to
5b57ece
Compare
obulat
force-pushed
the
use_cookie_for_layout
branch
4 times, most recently
from
1995b4f to
253c460
Compare
obulat
requested review from
zackkrida and
dhruvkb
and removed request for
a team
1 task
obulat
force-pushed
the
add/ui_state_cookie
branch
2 times, most recently
from
d1cc631 to
9b599fa
Compare
obulat
force-pushed
the
use_cookie_for_layout
branch
2 times, most recently
from
7737b96 to
55136dd
Compare
zackkrida
reviewed
Nov 30, 2022
zackkrida
approved these changes
Nov 30, 2022
There was a problem hiding this comment.
This looks really good. LGTM! Left some minor suggestions.
One thing I observed is that the new footer still flashes with the wrong layout, because it uses JS to set classes to determine its layout, since it basically needs 'container query'-like behavior to account for using the correct layout when the filter sidebar is open.
We should create a new issue to fix that after this lands.
sarayourfriend
approved these changes
Nov 30, 2022
| const route = useRoute() | ||
|
|
||
| const isMinScreenMd = inject(IsMinScreenMdKey) | ||
| const { all: allPages, current: currentPage } = usePages(true) |
There was a problem hiding this comment.
Is moving this one line down significant? Just want to check to understand why it's necessary if it was significant.
Comment on lines
+59
to
+66
| desktopBreakpoints(): Breakpoint[] { | ||
| const featureFlagStore = useFeatureFlagStore() | ||
| const isNewHeaderEnabled = computed(() => | ||
| featureFlagStore.isOn('new_header') | ||
| ) | ||
| return isNewHeaderEnabled.value | ||
| ? desktopBreakpoints | ||
| : [...desktopBreakpoints, 'md'] | ||
| }, |
There was a problem hiding this comment.
Do we need to maintain a list of desktop breakpoints or just the smallest desktop breakpoint? The method that maintains isDesktopLayout looks like it could just be a computed property something like: isDesktopLayout = () => isBreakpoint(smallestDesktopBreakpoint). Would it simplify anything to do it that way rather than manually maintaining the isDesktopLayout in the state? It is derived state so it seems prime for a computed.
Could you clarify which point that is? |
|
@obulat I suspect Sara is referring to this comment about '>' vs. '>=': |
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
Co-authored-by: Zack Krida <zackkrida@pm.me>
Co-authored-by: Zack Krida <zackkrida@pm.me>
obulat
force-pushed
the
use_cookie_for_layout
branch
from
b4f993c to
ac98453
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #975 by @sarayourfriend
Fixes #977 by @sarayourfriend
Fixes #885 by @sarayourfriend
Description
This PR replaces the
isMinScreencomposable used to detect whether we should use the desktop or mobile layout with the state values saved in theuistore (in PR #1955). This removes flickering during the re-rendering for most of the users that were caused by first rendering the mobile UI and changing it into a desktop layout if the screen is wide enough.The two layouts (
defaultandblank) useisMinScreencomposable to update the cookie if the screen width changes. This is done in theuseLayoutcomposable.This PR also refactors the code that shows or hides the filters. Instead of the localStorage, it uses the UI cookie for saving the state of the filters. By default, we always have the filters open on a desktop layout. If the filters are dismissed on a desktop layout, this is saved in a cookie.
Screenshots
Currently, when the search page is loaded client-side, it renders the correct layout when loading on desktop. However, when loading the same page on the server-side, it loads the mobile layout first and turns it into the desktop layout.
With a cookie-based approach, the mobile layout is loaded on desktop width only on the first-time load. All subsequent loads with the correct cookie are done with the correct layout.
Without cookie
On SSR, the homepage content switcher is shown after the page is rendered. The search page on CSR is loaded with the correct layout. On SSR (after page reload), the mobile header is loaded first (with a smaller content switcher button, and no page menu button), and then replaced with the desktop one.
Current.layout.mp4
With cookie
On SSR, the desktop content switcher is shown on the page load. The search page on CSR is loaded with the correct layout. On SSR, the search page is loaded with the correct layout because the cookie has been set already. There is a problem with the sidebar showing only after all of the rendering is done. This is caused by using Teleport for the sidebar content. I'll open a new issue to remove it.
Cookie-based.layout.mp4
Testing Instructions
Open 'http://localhost:8443/search/?q=cat' on the desktop width. You can throttle the network speed to better see the changes in this PR:
Instead of first loading the mobile layout, and then replacing it with the desktop layout, you get the desktop layout right away (or on the second load, after the cookie has been saved).
You can also try opening the site on a narrow screen, see the saved cookie, and then open the site in a different tab on a wide screen, and see that the cookie changes, and the layout is updated.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin