Add pink border to VSearchButton on group hover #1286

obulat · 2022-04-16T06:54:25Z

Fixes

Description

This PR adds a pink border on VSearchBar group hover so that the normal gray border doesn't make the button look small.
It also:

converts VSearchBar to TS
removes the duplicated VSearchBar story
adds visual regression testing snapshots for the header (following the conversation in Remove mob breakpoint and simplify VFilterButton #1275)
adds some missing tapes, and also adds the method to the end of the tape names if it's not GET.

Testing Instructions

Hover over the search bar in the header. The search button should always look like it has the same height as the search bar.

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.

zackkrida · 2022-04-16T16:10:43Z

@obulat I believe you've removed the actual fix and only kept the tests in this PR!

This reverts commit c0fa198.

obulat · 2022-04-17T02:29:33Z

@obulat I believe you've removed the actual fix and only kept the tests in this PR!

Yes! I wanted to see what the CI would show for the failing screenshot tests :) I remember seeing the two screenshots side by side showing the difference between the expected and the actual screenshot, but ~~I couldn't see it here~~ 🤷 Edit: Oh, there were no previous screenshots, so it's normal not to see the difference between them and new screenshots!
Well, the tests are still failing because 130 pixels (ratio 0.01 of all image pixels) are different :)

I planned to look at the CI test results and revert the change back, and thought that no one will see it on a weekend before traveling 😁

sarayourfriend

This looks so much better but there's a very slight gap still between the button and the search area on the leading edge where it connects to the search area's outline:

Approving because it's minor and we can solve in a separate PR if you want. Thanks for fixing this! It was bugging me today during our bug bash 😁

obulat · 2022-04-20T09:46:39Z

src/components/VInputField/VInputField.vue

@@ -1,6 +1,6 @@
 <template>
  <div
-    class="input-field group flex flex-row items-center hover:bg-dark-charcoal-06 focus-within:bg-dark-charcoal-06 group-hover:bg-dark-charcoal-06 p-0.5px focus-within:p-0 border focus-within:border-1.5 border-dark-charcoal-20 rounded-sm overflow-hidden focus-within:border-pink"
+    class="input-field group flex flex-row items-center focus-within:bg-dark-charcoal-06 group-hover:bg-dark-charcoal-06 p-0.5px focus-within:p-0 border focus-within:border-1.5 border-dark-charcoal-20 rounded-sm overflow-hidden focus-within:border-pink"


Background color is already applied on hover by group-hover:bg-dark-charcoal-06 class.

obulat · 2022-04-20T09:47:49Z

tsconfig.json

@@ -52,6 +52,8 @@
    "src/components/VNotificationBanner.vue",
    "src/components/VMigrationNotice.vue",
    "src/components/VVariations.vue",
+    "src/components/VInputField/VInputField.vue",
+    "src/components/VHeader/VSearchBar/VSearchBar.vue",


I did not add VSearchButton here because it depends on a couple of composables that are being converted in other PRs.

sarayourfriend · 2022-04-20T13:43:54Z

src/components/VHeader/VSearchBar/VSearchBar.vue

@@ -32,6 +32,7 @@ import { useMatchHomeRoute } from '~/composables/use-match-routes'
 import VInputField from '~/components/VInputField/VInputField.vue'
 import VSearchButton from '~/components/VHeader/VSearchBar/VSearchButton.vue'

+const SIZES = ['small', 'medium', 'large', 'standalone']


Given these are passed straight through to the VInputField, could we import the array of sizes from there instead?

Thank you for this suggestion! I have forgot that we are re-using the same array. Edited in the last commit.

Make snapshots for whole header Scroll header by focusing on Load more button Use data-testid to find header (out of 3 header elements) Add method to tape name if it's not `GET` Skip some tests for faster debugging Add missing tapes Remove `.skip` for visual regression tests Add snapshots

krysal

The button looks better. Thanks for all the improvements in the tests side! :)

krysal · 2022-04-22T09:30:43Z

test/proxy.js

+    let suffix = `${tape.req.headers.connection}`
+    if (tape.req.method !== 'GET') {
+      suffix = `${suffix}_${tape.req.method}`
+    }


Nice addition 💯

Add pink border to VSearchButton on group hover

53dc5a8

obulat added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 🕹 aspect: interface Concerns end-users' experience with the software labels Apr 16, 2022

obulat requested a review from a team as a code owner April 16, 2022 06:54

obulat requested review from krysal and sarayourfriend April 16, 2022 06:54

obulat added 4 commits April 16, 2022 15:43

Try adding visual regression tests

d51a556

Add hover

e9c6614

Add snapshots

f5449a9

Test visual regression tests :)

c0fa198

Revert "Test visual regression tests :)"

d004845

This reverts commit c0fa198.

Add missing test tapes

70d0107

sarayourfriend approved these changes Apr 19, 2022

View reviewed changes

obulat added 2 commits April 20, 2022 12:38

Unify SearchBar stories and TS-ify

73bf4b2

TS-ify VSearchBar

3a18fd7

obulat commented Apr 20, 2022

View reviewed changes

sarayourfriend reviewed Apr 20, 2022

View reviewed changes

Reuse VInputField FIELD_SIZES constant in VSearchBar

80d1a03

obulat force-pushed the fix/search_button branch from 5ff1adb to 1349bbb Compare April 21, 2022 06:52

obulat added 2 commits April 21, 2022 14:58

Add snapshots

8a46962

obulat force-pushed the fix/search_button branch from 11e91ea to 8a46962 Compare April 21, 2022 11:59

krysal approved these changes Apr 22, 2022

View reviewed changes

obulat merged commit 46d58db into main Apr 22, 2022

obulat deleted the fix/search_button branch April 22, 2022 10:36

obulat mentioned this pull request May 13, 2022

Add types to composables/use-match-routes.js #958

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pink border to VSearchButton on group hover #1286

Add pink border to VSearchButton on group hover #1286

obulat commented Apr 16, 2022 •

edited

Loading

zackkrida commented Apr 16, 2022

obulat commented Apr 17, 2022 •

edited

Loading

sarayourfriend left a comment

obulat Apr 20, 2022

obulat Apr 20, 2022

sarayourfriend Apr 20, 2022

obulat Apr 20, 2022

krysal left a comment

krysal Apr 22, 2022

Add pink border to VSearchButton on group hover #1286

Add pink border to VSearchButton on group hover #1286

Conversation

obulat commented Apr 16, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

zackkrida commented Apr 16, 2022

obulat commented Apr 17, 2022 • edited Loading

sarayourfriend left a comment

Choose a reason for hiding this comment

obulat Apr 20, 2022

Choose a reason for hiding this comment

obulat Apr 20, 2022

Choose a reason for hiding this comment

sarayourfriend Apr 20, 2022

Choose a reason for hiding this comment

obulat Apr 20, 2022

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

krysal Apr 22, 2022

Choose a reason for hiding this comment

obulat commented Apr 16, 2022 •

edited

Loading

obulat commented Apr 17, 2022 •

edited

Loading