Fix VIconButton default variant #1255

sarayourfriend · 2022-04-11T17:05:51Z

Fixes

Description

Fixes the problem in the linked issue by fixing the root cause in the VIconButton component. Also does some TypeScript stuff to make the VButton easier to use with TypeScript and able to type things like VIconButton that "spread props" to VButton.

Testing Instructions

Check out the VIconButton implementations in the app and ensure they look good:

PlayPause button
Content report form close button
License explanation tooltip in the filter list
Global audio player close button

I could add visual-regression tests for each of these but in the interest of expediency I only added it for the particular button in question.

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.

obulat · 2022-04-11T17:25:14Z

src/components/VButton.vue

@@ -95,7 +101,7 @@ const VButton = defineComponent({
    as: {
      type: String as PropType<ButtonForm>,
      default: 'button',
-      validate: (val: ButtonForm) => buttonForms.includes(val),
+      validate: (val: unknown) => buttonForms.includes(val as ButtonForm),


I only changed this because I was trying to get the ExtractPropTypes to work correctly with props that have default values. I'll set it back, there's no functional or operational difference either way.

(Though the unknown parameter type is technically more correct).

It just doesn't matter because we don't call validate directly so the type checking in this function is basically a "whatever" kind of thing.

obulat · 2022-04-11T17:28:04Z

src/components/VAudioTrack/VPlayPause.vue

@@ -36,22 +36,20 @@ export default defineComponent({
     * the current play status of the audio
     */
    status: {
-      type: String,
-      validator: (val) => ['playing', 'paused', 'played'].includes(val),
+      type: String as PropType<'playing' | 'paused' | 'played'>,


We could use the type from active media store here:

openverse-frontend/src/stores/active-media.ts

Line 7 in bccbd13

type MediaStatus = 'ejected' | 'playing' | 'paused' // 'ejected' means player is closed

It's a different status, this one has "played" instead of "ejected". IIRC there is some distinction, perhaps one for when the player is closed ("ejected") and the other for when the audio has simply finished playing to the end (in which case we show the replay button).

Oh, thank you for explaining :)

obulat

The buttons' colors are fixed. There are some size inconsistencies with the mockups:

The close button on the License Popover is the correct size (40x40px), but it probably should be right at the top of the popover (without any padding/margin).
The close button on the content report form is too big (48x48 instead of 40x40)

Do you think we should open a new issue for that?

sarayourfriend · 2022-04-14T15:54:23Z

@obulat I can fix those here.

For the button position, I moved it because it looked awkward and wrong for it to not be vertically aligned with header text of the popover. cc @panchovm on that one.

fcoveram · 2022-04-14T18:21:53Z

In both the license Popover and the content report form, I see 48px as size for the button element. Is that for a reason? I am using a 24px size in the mockups to match other similar buttons.

Regarding the close button position in popovers, I have pending reviewing the popover style. Some cases have more content than others, and the vertical alignment can blend with the popover's title area. Here is an example.

For now, I would keep it top-right aligned with a top and right margin of 8px.

Also, notice that the resting style of the close button is dark-charcoal instead of dark-charcoal-70. I can create a ticket to add the state styles that interactive components have.

obulat · 2022-04-15T04:19:25Z

I think that the close buttons should be at least 40x40px, because on the mobile phones it's very difficult to click on buttons that are smaller than 40px. This is also an accessibility/SEO requirement that is checked by Google Lighthouse.
If you have a margin of 8px, the 24x24px button looks like it's a 40x40px button. We can have a button that is 40x40px right in the corner of the popover, that looks exactly as the designs, don't you think, @panchovm ?

krysal

LGTM!

krysal · 2022-04-21T13:28:20Z

src/components/VButton.vue

+export type ButtonProps = ProperlyExtractPropTypes<
+  NonNullable<typeof VButton['props']>
+>


Shouldn't this be after the declaration of VButton?

I couldn't decide 😅 I thought putting it before would make it more obvious where it was coming from for someone reading this and wondering how it worked 🤔 type declarations and references can appear in any order in a file so it doesn't cause any integrity issues.

fcoveram · 2022-04-21T13:37:50Z

The target size criterion requires a 48px size, but that is for AAA level. For AA it seems not necessary. Nonetheless, you are right about the size being too small. We already have a close component that has a 32px for the smallest size. We might go with that.

What do you think?

sarayourfriend requested a review from a team as a code owner April 11, 2022 17:05

sarayourfriend requested review from krysal and obulat April 11, 2022 17:05

openverse-bot added 🕹 aspect: interface Concerns end-users' experience with the software 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents labels Apr 11, 2022

obulat reviewed Apr 11, 2022

View reviewed changes

sarayourfriend force-pushed the fix/report-form-close-button branch from 83e0fe1 to 9de8a8e Compare April 14, 2022 09:36

obulat approved these changes Apr 14, 2022

View reviewed changes

sarayourfriend force-pushed the fix/report-form-close-button branch 3 times, most recently from 63c6952 to eb59273 Compare April 15, 2022 08:22

krysal approved these changes Apr 21, 2022

View reviewed changes

sarayourfriend added 5 commits April 22, 2022 14:44

Fix VIconButton default variant

a21953a

Undo unnecessary code changes

b0f9fae

Update POT

02da8ad

Fix close button position and color

2cc1213

Increase timeout to prevent flakiness

dadebee

sarayourfriend force-pushed the fix/report-form-close-button branch from eb59273 to dadebee Compare April 22, 2022 12:45

sarayourfriend merged commit cca06e9 into main Apr 22, 2022

sarayourfriend deleted the fix/report-form-close-button branch April 22, 2022 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VIconButton default variant #1255

Fix VIconButton default variant #1255

sarayourfriend commented Apr 11, 2022 •

edited

Loading

obulat Apr 11, 2022

sarayourfriend Apr 11, 2022

sarayourfriend Apr 11, 2022

sarayourfriend Apr 11, 2022

obulat Apr 11, 2022

sarayourfriend Apr 11, 2022

obulat Apr 13, 2022

obulat left a comment

sarayourfriend commented Apr 14, 2022

fcoveram commented Apr 14, 2022

obulat commented Apr 15, 2022

krysal left a comment

krysal Apr 21, 2022

sarayourfriend Apr 21, 2022

fcoveram commented Apr 21, 2022

Fix VIconButton default variant #1255

Fix VIconButton default variant #1255

Conversation

sarayourfriend commented Apr 11, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

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

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

sarayourfriend commented Apr 14, 2022

fcoveram commented Apr 14, 2022

obulat commented Apr 15, 2022

krysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcoveram commented Apr 21, 2022

sarayourfriend commented Apr 11, 2022 •

edited

Loading