Update banner styles #1184

sarayourfriend · 2022-03-25T10:59:47Z

Fixes

Fixes #1123 by @panchovm

Description

Primarily focused on updating the banner styles and unifying the implementations so that both have a close button. The unification effort is where most of the changes come from as well as some on-the-fly TypeScript conversion.

I'd like to split this PR into multiple parts but will be on vacation for the next week so just wanted to get my WIP up in case someone else wanted to pick it apart while I'm gone.

Changes proposed:

Convert use-storage and use-event-listener to TypeScript and simplify the initial value handling in use-storage.
Update local.ts to implement the full Storage API 1:1 instead of using a bespoke API. Also simplify the error handling in it.
Convert NotificationBanner to TypeScript and update it to be able to handle dismissal on its own rather than relying on the parent to handle it. This makes it easy to implement new dismissable banners without having to duplicate logic.
Update TranslationBanner to use the new NotificationBanner API and remove the unnecessary stuff from use-i18n-sync (which can probably be renamed now that nothing is being "synced")

Testing Instructions

Checkout the branch and run pnpm dev. Visit the site in English and verify there is no banner. Visit the site in Russian (ru) and verify there is a banner and that it can be dismissed and that it stays dismissed between requests (aside from the already existing problem that the banner is rendered SSR, to be fixed by the UI state cookie milestone).

Visit the site in another low-translation percent language like Portuguese (pt). Verify that a new banner shows with the correct information and that it is dismissable and stays dismissed. Verify that you can switch between Russian and Portuguese and get no banner.

Now do the same with two more low-translation-percent languages and do not dismiss the banners and ensure that both show the appropriate banner between page requests.

Note: All of the above could be encoded in e2e tests and I plan to do that when I get back from vacation and have time to implement additional tests for this on top of the already existing ones.

The CC referral banner is not yet implemented but testing it will go as follows:

Visit the site with the referrer=creativecommons.org query param and ensure that the banner shows and that it is dismissable. Ensure that it stays dismissed on subsequent requests to the site with the query param. (note again that this has not yet been implemented on this branch!).

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend · 2022-04-11T07:56:23Z

Blocked by #1241 for the tapes fix.

obulat · 2022-04-11T17:10:48Z

src/components/DownloadButton.vue

@@ -91,7 +91,7 @@ export default {
  },
  data() {
    const savedFormatExtension =
-      local.get(LS_DOWNLOAD_FORMAT_EXTENSION_KEY) ?? null
+      local.getItem(LS_DOWNLOAD_FORMAT_EXTENSION_KEY) ?? null


Would using useStorage composable be an overkill here?

I guess not, I'd just like to avoid doing that refactor in this PR as well if that's okay? We can open a separate issue for it 🙂

Sure, I just remembered looking at this PR previously and trying to understand the difference between the two. Then I saw that useStorage is actually using local internally

obulat · 2022-04-11T17:12:23Z

src/components/VTranslationStatusBanner.vue

+    v-if="needsTranslationBanner"
+    :id="bannerKey"
+    variant="informational"
+  >
    {{
      // eslint-disable-next-line @intlify/vue-i18n/no-raw-text
      '⚠️'


We could add this symbol to ignored strings that @krysal created.

Should we use that list for anything you think? The ones Krystle added made sense to me because they're used in a couple of places in the app, I think... Hopefully raw emoji aren't that common in the app 🤔

Now that you say so, I guess it does make sense to use that list for repeated strings, and not for single emojis :)

obulat

The button on the mobile version is positioned incorrectly: it should be at the end horizontally, but it shows at the bottom.

Figma mockup: https://www.figma.com/file/w60dl1XPUvSaRncv1Utmnb/Audio-Release?node-id=953%3A17185

sarayourfriend · 2022-04-11T18:03:34Z

@obulat thanks for catching that. I think it is due to the md: modifiers in VNotificationBanner. They're leftover from the previous, specific implementation. Removing them now to see if it fixes it.

obulat · 2022-04-11T18:06:12Z

src/layouts/blank.vue

@@ -1,6 +1,6 @@
 <template>
  <div>
-    <VMigrationNotice v-show="isReferredFromCc" />
+    <VMigrationNotice v-if="isReferredFromCc" />


Would this cause client-server mismatch? Often when you use v-if and the server state is different from the client state (i.e., something depends on the client state such as localStorage), the mismatch will cause 'hydration bail' and full re-rendering on the client.

Ah, I thought I remembered there being a good reason for it being v-show. I'll put it back.

We can move it to v-if if we want when we implement the ui state cookie.

obulat

There needs to be some more logic checking the value in localStorage to show/hide the banner after it's been dismissed and the page is reloaded.

obulat · 2022-04-12T08:42:03Z

src/components/VTranslationStatusBanner.vue

@@ -1,5 +1,9 @@
 <template>
-  <VNotificationBanner v-show="!shouldHideBanner" @close="dismissBanner">
+  <VNotificationBanner
+    v-show="needsTranslationBanner"


The v-show logic here only uses the translation percentage, and doesn't check for the value in localStorage. This is why if you dismiss a banner, and refresh the page, you will see the banner again, and will not be able to dismiss it anymore.

That logic is handled in the VNotificationBanner component itself, with the local storage. But it seems like stacking v-show or something is messing with it. Thanks for catching this, I will debug it.

So I think the issue is that because we have a v-show on the external render of the component, and because v-show just uses CSS to show/hide the element, it's overriding the local storage handling inside the component because v-show="true" in the component rendering it, even though inside (I've confirmed) v-show="false".

The only thing I can think of is to forward the "v-show" variable as a new prop to be combined with the local storage variable. Hacky, but I guess it is necessary for v-show because of the way it works.

There are two layers of v-show now, in the TranslationBanner and in the NotificationBanner (one checks for local storage, the other - for translation percentage). Maybe they are cancelling each other out? It would be nice to combine them, use the outer v-show value as a prop to NotificationBanner, maybe?

The comment was too late, you've already implemented the fixes 🤣

obulat

Works! Thank you for lots of added tests.

sarayourfriend · 2022-04-15T05:52:31Z

@dhruvkb can you take a look at this if you get a chance today? Thanks!

dhruvkb

LGTM, love the tryUse function. Why are the checks failing?

sarayourfriend · 2022-04-15T07:34:45Z

It must be the button variations. Looking into it now. Thanks for the review!

sarayourfriend requested a review from a team as a code owner March 25, 2022 10:59

sarayourfriend requested review from obulat and dhruvkb and removed request for a team March 25, 2022 10:59

sarayourfriend marked this pull request as draft March 25, 2022 11:02

Base automatically changed from fix/missing-nuxt-types to main March 25, 2022 14:16

sarayourfriend force-pushed the update/banner-styles#1123 branch from 1435dc2 to 602d1da Compare April 11, 2022 04:50

sarayourfriend marked this pull request as ready for review April 11, 2022 07:56

obulat reviewed Apr 11, 2022

View reviewed changes

obulat suggested changes Apr 11, 2022

View reviewed changes

sarayourfriend force-pushed the update/banner-styles#1123 branch from c252abb to b5f8785 Compare April 11, 2022 17:59

obulat reviewed Apr 11, 2022

View reviewed changes

obulat suggested changes Apr 12, 2022

View reviewed changes

obulat approved these changes Apr 12, 2022

View reviewed changes

sarayourfriend force-pushed the update/banner-styles#1123 branch from 24b4986 to d75f171 Compare April 15, 2022 05:24

dhruvkb approved these changes Apr 15, 2022

View reviewed changes

sarayourfriend added 6 commits April 15, 2022 10:07

Add multiple styles to VNotificationBanner

c6c18cf

Remove stray comment

0fc7f8e

Fix tsconfig includes

ea81dbe

Fix text and button colors

233071e

Fix visibility and selection of banner close buttons

1daf4d1

Update POT file

3c8d0d5

sarayourfriend added 9 commits April 15, 2022 10:08

Fix mobile icon placement and size

133b57b

Fix stacked v-show

8636b34

Update POT

61c877c

Update homepage vr tests

41887fd

Remove unnecessary prop and add VVariations

0397973

Remove redundant VVariations component

0d98285

Fix clashing class props

879a193

Use smaller button always

dbf18f9

Slightly increase timeout to prevent flakiness

4e147e2

sarayourfriend force-pushed the update/banner-styles#1123 branch from 5069102 to 4e147e2 Compare April 15, 2022 08:11

sarayourfriend merged commit 983495b into main Apr 15, 2022

sarayourfriend deleted the update/banner-styles#1123 branch April 15, 2022 08:20

sarayourfriend mentioned this pull request Feb 22, 2023

RTL-ify the translation notification banner border WordPress/openverse#572

Closed

1 task

zackkrida added 🧰 goal: internal improvement Improvement that benefits maintainers, not users 💻 aspect: code Concerns the software code in the repository labels May 10, 2022

This was referenced May 13, 2022

Add types to composables/use-event-listener.js #952

Closed

Add types to composables/use-storage.js #954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update banner styles #1184

Update banner styles #1184

sarayourfriend commented Mar 25, 2022 •

edited

Loading

sarayourfriend commented Apr 11, 2022

obulat Apr 11, 2022

sarayourfriend Apr 11, 2022

obulat Apr 11, 2022

obulat Apr 11, 2022

sarayourfriend Apr 11, 2022

obulat Apr 11, 2022

obulat left a comment •

edited

Loading

sarayourfriend commented Apr 11, 2022

obulat Apr 11, 2022

sarayourfriend Apr 11, 2022

sarayourfriend Apr 11, 2022

obulat left a comment

obulat Apr 12, 2022

sarayourfriend Apr 12, 2022

sarayourfriend Apr 12, 2022

obulat Apr 12, 2022

obulat Apr 12, 2022

obulat left a comment

sarayourfriend commented Apr 15, 2022

dhruvkb left a comment •

edited

Loading

sarayourfriend commented Apr 15, 2022

Update banner styles #1184

Update banner styles #1184

Conversation

sarayourfriend commented Mar 25, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat left a comment • edited Loading

Choose a reason for hiding this comment

sarayourfriend commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

sarayourfriend commented Apr 15, 2022

dhruvkb left a comment • edited Loading

Choose a reason for hiding this comment

sarayourfriend commented Apr 15, 2022

sarayourfriend commented Mar 25, 2022 •

edited

Loading

obulat left a comment •

edited

Loading

dhruvkb left a comment •

edited

Loading