This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Use cookie for banners state #1957
Merged
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
obulat
added
🟧 priority: high
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1957 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. |
obulat
force-pushed
the
use_cookie_for_banners
branch
from
de0b65a
to
67df089
Compare
|
Size Change: -5.09 kB (-1%) Total Size: 808 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
use_cookie_for_banners
branch
from
67df089
to
0dd90f9
Compare
obulat
force-pushed
the
add/ui_state_cookie
branch
4 times, most recently
from
dd5279c
to
630d501
Compare
obulat
force-pushed
the
use_cookie_for_banners
branch
from
0dd90f9
to
2aaef47
Compare
zackkrida
suggested changes
Nov 15, 2022
Update the ui store
obulat
force-pushed
the
use_cookie_for_banners
branch
from
2aaef47
to
252177c
Compare
Sorry, I should have drafted this PR until I updated it with the latest changes.
Thanks for catching, it was caused by the |
sarayourfriend
suggested changes
Nov 21, 2022
| /** | ||
| * We only hide the enabled banner if there is a cookie. | ||
| */ | ||
| const shouldShow = ref(!uiStore.isBannerDismissed(props.id)) |
There was a problem hiding this comment.
This is definitely just a nit about how to manage refs and store-derived data: Would it make sense to call isBannerDismissed inside of a computed callback? That way the dismissBanner function doesn't have to futz with updating the ref:
const shouldShow = computed(() => !uiStore.isBannerDismissed(props.id))
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
obulat
force-pushed
the
use_cookie_for_banners
branch
from
4022ebd
to
6a1e80f
Compare
While adding some code review suggestions, I realized that just like the layout properties, it is best to keep everything inside the |
sarayourfriend
approved these changes
Nov 24, 2022
Comment on lines
+23
to
+25
| VMigrationNotice: () => import('~/components/VBanner/VMigrationNotice.vue'), | ||
| VTranslationStatusBanner: () => | ||
| import('~/components/VBanner/VTranslationStatusBanner.vue'), |
There was a problem hiding this comment.
Does this work asynchronously? 😮 Does it avoid including unused banners?
There was a problem hiding this comment.
It allows us to lazily load components. It's probably not as important for small components. We can also add the webpack comment to allow prefetching, although for the banners it doesn't make sense: the banner will only be displayed if the user goes to a language with fewer translated strings, but we would prefetch the code for everyone?
More info about it here:
https://vueschool.io/articles/vuejs-tutorials/lazy-loading-individual-vue-components-and-prefetching/
It would be nice to set up a way of measuring performance to see if things like this are really necessary or not.
| @@ -0,0 +1,48 @@ | |||
| <template> | |||
There was a problem hiding this comment.
I really like this abstraction 🚀
|
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 |
zackkrida
reviewed
Nov 30, 2022
zackkrida
reviewed
Nov 30, 2022
zackkrida
reviewed
Nov 30, 2022
zackkrida
reviewed
Nov 30, 2022
zackkrida
reviewed
Nov 30, 2022
Co-authored-by: Zack Krida <zackkrida@pm.me>
Co-authored-by: Zack Krida <zackkrida@pm.me>
Co-authored-by: Zack Krida <zackkrida@pm.me>
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 #976 by @sarayourfriend
Description
This PR adds a UI cookie to save the IDs of banners that have been dismissed by the user.
Testing Instructions
Try opening
http://localhost:8443/ar/search?q=cat&referrer=creativecommons.org. You should see 2 banners at the top of the window. If you dismiss one of them and reload, it should not re-appear. If you clear the cookies for localhost and reload, it should re-appear.Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin