Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Use links instead of buttons for header search type switcher #1131

Merged
merged 7 commits into from Mar 22, 2022

Conversation

obulat
Copy link
Contributor

@obulat obulat commented Mar 17, 2022

Fixes

Fixes #1130 by @obulat

Note: This PR works for adding links that retain common filter query parameters, but for correctly handling the search type specific query parameters it is dependent on #1040 (it needs an action that is only available in that PR). Since these two PRs are interdependent, it is best to approve and merge them together, or add TODO into another PR once one of them is merged.

Description

This PR makes the search type switcher use VLink when used in the header, and keep the current button status when used in the homepage searchbar.
This makes the header links more semantic (and also sets the aria-current="page" for the link to currently selected search type), and simplifies the search type handling.
It also fixes the e2e tests that are failing in #1040, and are currently disabled.

Testing Instructions

Run the app using pnpm dev, search for something from the homepage.

Switching search type from the homepage search bar should work as usual, and should not redirect to another page, because it's not using an <a> link.

Selecting a search type from the header should redirect you, as usual, and update the url path. You can see in the devtools that the search type switcher is using <a> links.

Currently, the link will only retain the filter query parameters that are common for all search types (license, license types, search by creator).
After the search store change is merged, You should also see that the query in those links retains the filter parameters that that type can use, and removes the parameters that are not appropriate. For example, if on the image page you check the 'jpg' file type filter and 'cc0' license filter, the link to 'all_content' type should only contain 'cc0' in the parameter, and discard the 'jpg' parameter.

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
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 obulat added the 💻 aspect: code Concerns the software code in the repository label Mar 17, 2022
@obulat obulat requested a review from a team as a code owner March 17, 2022 07:22
@obulat obulat requested review from krysal and dhruvkb March 17, 2022 07:22
@dhruvkb dhruvkb added this to Needs review in Openverse PRs Mar 17, 2022
@obulat obulat added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature labels Mar 17, 2022
@obulat obulat force-pushed the pinia_search_convert_search_store branch 2 times, most recently from c5a2889 to 1c25a5b Compare March 18, 2022 02:15
@obulat obulat changed the title Use <a> links for header search type switcher Use links instead of buttons for header search type switcher Mar 18, 2022
@obulat obulat force-pushed the use_links_for_header_search_type_switcher branch from 0ab1557 to 710e40c Compare March 18, 2022 05:52
@obulat obulat changed the base branch from pinia_search_convert_search_store to main March 18, 2022 05:52
@kb-0311 kb-0311 mentioned this pull request Mar 19, 2022
7 tasks
# Conflicts:
#	src/locales/po-files/openverse.pot
@@ -46,6 +47,10 @@ export default defineComponent({
required: true,
validator: (val) => supportedSearchTypes.includes(val),
},
placement: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned earlier, I think a name that indicates the internal component behavior is better than one that refers to the external context. placement might be better as useLinks or some better name.

Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works really well! I made some optional suggestions to code style but otherwise lgtm.

Copy link
Contributor

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I wrote a comment last week and then forgot to submit this review, sorry! Zack mentioned something similar and probably better, just using a specific prop name instead of something contextual! Easier to test in Storybook that way as well 🙂 Kudos Zack!

@@ -30,16 +33,33 @@ const propTypes = {
itemId: { type: Number, required: true },
selected: { type: Boolean, default: false },
icon: { type: String, required: true },
isInHeader: { type: Boolean, default: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This props like these I always wonder whether continue to drill them down or switching to using provide/inject would make more sense. I guess this is probably easier to test. It also crosses my mind "at what point does one decide to turn the enum into a boolean" in this case. For example, why pass the boolean here instead of passing placement directly which can then be evaluated by the component?

I don't have good answers for these though 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this comment, @sarayourfriend ! I have also thought about replacing props drilling with provide/inject, but decided against it for one single prop.

Openverse PRs automation moved this from Needs review to Reviewer approved Mar 21, 2022
@obulat obulat merged commit f09e1d9 into main Mar 22, 2022
Openverse PRs automation moved this from Reviewer approved to Merged! Mar 22, 2022
@obulat obulat deleted the use_links_for_header_search_type_switcher branch March 22, 2022 03:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature 🟨 priority: medium Not blocking but should be addressed soon
Projects
No open projects
Openverse PRs
  
Merged!
Development

Successfully merging this pull request may close these issues.

Search type switcher items in the header should use a link instead of a button
3 participants