VLogoLoader #448

zackkrida · 2021-11-25T02:14:11Z

Fixes

Description

Adds a new VLogoLoader component and some supporting composables:

One for using event listeners (no longer being used, so I could remove)
One for using media queries (added by @obulat modified by me)
One for using prefers-reduced-motion

The loader itself is an aria-hidden, visual enhancement to the site. It does not have any inherent accessibility qualities, nor is it focusable. Originally I thought this component should be an ARIA Live Region¹ but after some research that will be a role better served by our actual search grid, which can announce it's own loading status, the # of newly added results after the user hits the 'load more' button, and so on. I'll make a new ticket soon to spec out those improvements.

Testing Instructions

This component can be tested with npm run storybook and viewing the VLogoLoader story. There are a few things you can do:

Toggle the status prop from "loading" to "idle" with the handles
Set prefers-reduced-motion in your browser while in the "loading" status, observe the change in animation to a more subtle, non-moving effect. This is currently a gentle pulse from full to partial opacity. @panchovm might have a better idea for a subtle animation that doesn't add movement, but still visually conveys to the user that loading is occurring. Prefers reduced motion !== wants no motion. Here's a quick video on how to test for prefers reduced motion:

CleanShot.2021-12-02.at.19.26.37-converted.mp4

Follow-up questions

Please suggest any test cases I should add. I admittedly don't love unit testing of front-end components and am always looking for improvements there.
The component is extremely small in terms of pixel values in the mockups (cc @panchovm), with the logo being 20px tall with 14px of padding surrounding. This has been increased to 32px tall with 16px of padding in my component. Otherwise the Openverse logo is barely legible, even on my 5k monitor.

My sizing

Figma sizing
The mobile menu includes the logotext "Openverse", I think that can simply exist next to this loader but perhaps be wrapped in the same <NuxtLink /> component.

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.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions ↩

src/components/VLogoLoader/VLogoLoader.vue

src/composables/use-event-listener.js

src/composables/use-media-query.js

zackkrida · 2021-12-01T21:59:52Z

Note, the current aria live region implementation is incorrect 😄 but will be revised soon.

Edit: It doesn't exist anymore, and is unnecessary for this component.

obulat · 2021-12-03T11:49:20Z

src/components/VLogoLoader/VLogoLoader.vue

+   * @param {import('@nuxtjs/composition-api').SetupContext} context
+   */
+  setup() {
+    const defaultWindow = typeof window !== 'undefined' ? window : undefined


The default window is being used in several places now to ensure that composables work correctly with SSR. Is there a way to extract this code somehow?

I was thinking it could be the default value in the hook, at least. Not sure if it'll work, I'll test

I don't think you can use a default value as it'll try to evaluate window even when it's undefined and throw an error.

src/components/VLogoLoader/VLogoLoader.vue

Co-authored-by: Olga Bulat <obulat@gmail.com>

obulat

The Logo loader component is awesome!

krysal

Looks amazing! You can see the attention to details, really nice ✨

src/components/VLogoLoader/meta/VLogoLoader.stories.js

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

sarayourfriend

Nice! The CSS animations are magic to me 🤩 Just a couple documentation things but otherwise looks great!

With respect to the "pulsing" reduced motion version... I have decent vision and I can barely tell anything is happening. In fact, it might as well not even pulse if it's that subtle.

src/components/VLogoLoader/VLogoLoader.types.js

sarayourfriend · 2021-12-03T16:19:41Z

src/components/VLogoLoader/VLogoLoader.vue

+   * @param {import('@nuxtjs/composition-api').SetupContext} context
+   */
+  setup() {
+    const defaultWindow = typeof window !== 'undefined' ? window : undefined


I don't think you can use a default value as it'll try to evaluate window even when it's undefined and throw an error.

src/composables/use-event-listener.js

sarayourfriend · 2021-12-03T16:22:33Z

src/composables/use-event-listener.js

+    watch(target, (value, oldValue) => {
+      oldValue?.removeEventListener(event, handler)
+      value?.addEventListener(event, handler)
+    })


Would it be better to use onInvalidate to remove the event listener?

Suggested change

watch(target, (value, oldValue) => {

oldValue?.removeEventListener(event, handler)

value?.addEventListener(event, handler)

})

watch(target, (value, _, onInvalidate) => {

value?.addEventListener(event, handler)

onInvalidate(() => value?.removeEventListener(event, handler))

})

I'm not 100% sure there is a difference, I'll do some fiddling to test this out, but i think the watcher may never run on component dismount to remove the old listener but invalidate will run.

I do like your take on it! 😄

sarayourfriend · 2021-12-03T16:23:02Z

src/composables/use-event-listener.js

+  onBeforeUnmount(() => {
+    unref(target)?.removeEventListener(event, handler)
+  })


Oh wait, I see, I guess onInvalidate isn't necessary because the clean up happens in one place here?

test/unit/specs/components/v-logo-loader.spec.js

src/composables/use-media-query.js

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

fcoveram · 2021-12-03T16:40:43Z

It looks awesome. Great work.

I agree with @sarayourfriend as the fading effect is not very noticeable. And I would add that it might be unnecessary. The idea of motion reduction is great, but in this case, the animation is very subtle and lives in a context where the skeleton interface fading is more proper for showing the loading instance.

zackkrida · 2021-12-03T16:50:18Z

@panchovm that's a great point about the skeleton loaders. I think I will go with no animation then!

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

zackkrida · 2021-12-03T17:04:32Z

@sarayourfriend Let's follow up on the event listener code testing—it'd be nice to adopt that in more places in the future too.

zackkrida added 2 commits November 24, 2021 20:48

init

49ffdc1

Reduced motion support and basic state management

bdb85ad

zackkrida commented Nov 25, 2021

View reviewed changes

src/components/VLogoLoader/VLogoLoader.vue Outdated Show resolved Hide resolved

zackkrida commented Nov 25, 2021

View reviewed changes

src/components/VLogoLoader/VLogoLoader.vue Outdated Show resolved Hide resolved

zackkrida self-assigned this Nov 25, 2021

zackkrida added 🌟 goal: addition Addition of new feature 🕹 aspect: interface Concerns end-users' experience with the software 🟧 priority: high Stalls work on the project or its dependents labels Nov 25, 2021

obulat reviewed Nov 25, 2021

View reviewed changes

src/composables/use-event-listener.js Show resolved Hide resolved

obulat reviewed Nov 25, 2021

View reviewed changes

src/composables/use-media-query.js Outdated Show resolved Hide resolved

zackkrida added 5 commits December 1, 2021 15:19

Merge branch 'main' into v-logo-loader-fix-#371

f41a60a

Improve use-media-query types and add back useReducedMotion

15c0160

Add window management for reduced motion media query composable

4b151a2

Add initial tests

1db788a

Add aria live region support

dc02695

zackkrida added 4 commits December 2, 2021 19:03

Add loading story

861e866

Implement animations; remove ARIA; cleanup

2929fb2

Fix tests

afa7731

Remove aria todo test

3e62fcf

zackkrida marked this pull request as ready for review December 3, 2021 01:13

zackkrida requested a review from a team as a code owner December 3, 2021 01:13

zackkrida requested review from krysal, dhruvkb and obulat December 3, 2021 01:13

sarayourfriend self-requested a review December 3, 2021 02:00

obulat mentioned this pull request Dec 3, 2021

Header #462

Closed

6 tasks

obulat reviewed Dec 3, 2021

View reviewed changes

src/components/VLogoLoader/VLogoLoader.vue Outdated Show resolved Hide resolved

Update src/components/VLogoLoader/VLogoLoader.vue

f380dc1

Co-authored-by: Olga Bulat <obulat@gmail.com>

obulat approved these changes Dec 3, 2021

View reviewed changes

krysal approved these changes Dec 3, 2021

View reviewed changes

src/components/VLogoLoader/meta/VLogoLoader.stories.js Outdated Show resolved Hide resolved

Update src/components/VLogoLoader/meta/VLogoLoader.stories.js

65036fd

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

zackkrida added this to the Redesign milestone Dec 3, 2021

sarayourfriend reviewed Dec 3, 2021

View reviewed changes

zackkrida and others added 2 commits December 3, 2021 11:34

Update src/composables/use-event-listener.js

a5eb8db

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

Update src/composables/use-media-query.js

e060388

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

zackkrida and others added 3 commits December 3, 2021 11:57

Remove reduced motion animation

ae0201d

Add complementary test for reduced motion testing

9808f3f

Update src/components/VLogoLoader/VLogoLoader.types.js

12cedb4

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

zackkrida merged commit b8f5a39 into main Dec 3, 2021

zackkrida deleted the v-logo-loader-fix-#371 branch December 3, 2021 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VLogoLoader #448

VLogoLoader #448

zackkrida commented Nov 25, 2021 •

edited

Loading

zackkrida commented Dec 1, 2021 •

edited

Loading

obulat Dec 3, 2021

zackkrida Dec 3, 2021

sarayourfriend Dec 3, 2021

obulat left a comment

krysal left a comment

sarayourfriend left a comment

sarayourfriend Dec 3, 2021

sarayourfriend Dec 3, 2021

zackkrida Dec 3, 2021

sarayourfriend Dec 3, 2021

fcoveram commented Dec 3, 2021

zackkrida commented Dec 3, 2021

zackkrida commented Dec 3, 2021

VLogoLoader #448

VLogoLoader #448

Conversation

zackkrida commented Nov 25, 2021 • edited Loading

Fixes

Description

Testing Instructions

Follow-up questions

Checklist

Developer Certificate of Origin

Footnotes

zackkrida commented Dec 1, 2021 • edited Loading

obulat Dec 3, 2021

Choose a reason for hiding this comment

zackkrida Dec 3, 2021

Choose a reason for hiding this comment

sarayourfriend Dec 3, 2021

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Dec 3, 2021

Choose a reason for hiding this comment

sarayourfriend Dec 3, 2021

Choose a reason for hiding this comment

zackkrida Dec 3, 2021

Choose a reason for hiding this comment

sarayourfriend Dec 3, 2021

Choose a reason for hiding this comment

fcoveram commented Dec 3, 2021

zackkrida commented Dec 3, 2021

zackkrida commented Dec 3, 2021

zackkrida commented Nov 25, 2021 •

edited

Loading

zackkrida commented Dec 1, 2021 •

edited

Loading