Add Content Switcher component #607

obulat · 2022-01-14T14:02:11Z

Fixes

Fixes #485 by @zackkrida
This PR adds the Content Switcher to the Header. This is a new version of #591 that does not have merge conflicts. It is also based off of the filter_sidebar branch, so it shouldn't be merged before that.

Description

The VContentSwitcher component combines the Internal Page menu and the Content type switcher.
There is also a VPageMenu component used on internal pages (eg. About), where only the page menu, and not content switcher, are used.

This component turned out to be very complex.
On mobile screens and on initial SSR, the page menu button does not show, and clicking the content switcher button opens a modal with the Content types menu and the Pages menu.
On larger screens, clicking the Pages menu button (ellipsis icon) opens up a Popover with links to internal pages, and clicking the Content type button opens up a Popover with different content types available. Clicking on the items changes the search content type, and closes the popover.

Issues

There is a problem with tabbing through the items in the mobile modal. I am not sure why they can't be tabbed through, it worked previously.

Testing Instructions

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-01-14T17:40:36Z

krysal

This is looking really good! It's noticeable that these components are broad in functionality and styles variants. I want to check some files in more detail to learn more from your approach :) but at a first glance seems pretty complete.

I'm just seeing that the VPageMenu doesn't close after clicking on a link, it effectively navigates but the popover/full page modal keeps open, one has to click outside/close the modal to see the target page.

krysal · 2022-01-17T16:32:11Z

src/composables/use-scroll.js

+    })
+  }
+
+  return { y, isScrolled }


Doesn't this file serve the same purpose as use-window-scroll.js?

Their purposes are similar. At first, we were scrolling the whole window, but then we updated the layout so that it consists of a grid, in which the main content and the sidebar can be scrolled separately. This means that we can no longer detect if the header has scrolled or not by using window's scroll offset, and need to get the main content scrollTop instead. I did not remove the use-window-scroll.js just yet. But if it turns out after all the redesign work that we don't, we should delete use-window-scroll.js.

sarayourfriend

Just one blocker on the areQueriesEqual function and an invalid object literal comparison, but otherwise tests well and LGTM! Nice work!

src/components/VContentSwitcher/VContentTypes.vue

sarayourfriend · 2022-01-18T15:08:33Z

src/components/VContentSwitcher/VMobileMenuModal.vue

+      emit('select', item)
+    }
+
+    const isLinkExternal = (item) => !item.link.startsWith('/')


Nice! This would be something we could add to the VButton component as well 😮

sarayourfriend · 2022-01-18T15:17:57Z

src/composables/use-content-type.js

+  audio: audioIcon,
+  image: imageIcon,
+}
+export default function useContentType() {


I really like this way of encapsulating parts of Vuex into a composable. It feels really clean and creates a much nicer API for the components to work with. As a bonus I suspect this would make it easier to switch to pinia in the future.

sarayourfriend · 2022-01-18T15:21:10Z

src/store/search.js

@@ -346,7 +346,9 @@ const actions = {
      }
      commit(CLEAR_OTHER_MEDIA_TYPE_FILTERS, { searchType })
    }
-    await dispatch(UPDATE_QUERY_FROM_FILTERS, queryParams)
+    if (queryParams !== {}) {


This python-ism won't work in JavaScript 😢 If we need to test for an empty object there are two ways we could do it. Either we could test Object.keys(...).length and check it for 0 (which I'm assuming has some caveats that I can't think of, it seems like it should work fine) or we could use the lodash.isEmpty function.

sarayourfriend · 2022-01-18T15:25:15Z

src/utils/search-query-transform.js

+export const areQueriesEqual = (oldQuery, newQuery) => {
+  for (let key of Object.keys(oldQuery)) {
+    if (oldQuery[key] !== newQuery[key]) {
+      return false
+    }
+  }
+  return true
+}


Maybe this doesn't matter for queries but this wouldn't work if newQuery has keys that aren't present in oldQuery. For example, this will return true for:

areQueriesEqual({}, { foo: 'bar' })

We could use lodash.isEqual or else do something like is described in this SO answer after converting both arguments to Map objects or adapting that solution to just work on regular objects and avoid the conversion.

@sarayourfriend, I added lodash.isEqual. I was using this comparison because the queries were supposed to have the same structure, and the same number of properties. In fact, we probably won't need this comparison after the All content page is actually different from the Image page.

Gotcha! I figured that maybe this specific consideration didn't matter for queries for that reason. Either way works for me!

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

sarayourfriend

LGTM!

krysal

Works perfectly on a desktop layout! This is an incredible work 🌟

I noticed some issues on mobile, like the label of the content switcher button not changing, or the Openverse logo text not hiding on non-search pages, but I think those can be safely addressed in separate PR.

CleanShot.2022-01-18.at.12.33.15.mp4

# Conflicts: # src/locales/scripts/wp-locales.json

* Open filters in sidebar or modal from the header * Add layout changes from add/content_switcher * Add content switcher and internal page menu * Simplify header menu component * Remove NuxtLink from content type popover * Make ContentSwitcher more flexible * Set active content type using prop * Reuse components * Fix close button style * Small fixes to imports * Remove unused props * Update src/components/VContentSwitcher/VContentTypes.vue Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Fix query comparisons * Fix mobile content switcher button not changing Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

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

obulat added this to the Redesign milestone Jan 14, 2022

obulat self-assigned this Jan 14, 2022

obulat marked this pull request as ready for review January 14, 2022 16:21

obulat requested a review from a team as a code owner January 14, 2022 16:21

obulat requested review from krysal and dhruvkb and removed request for a team January 14, 2022 16:21

zackkrida mentioned this pull request Jan 14, 2022

Content Switcher Component #591

Closed

15 tasks

obulat mentioned this pull request Jan 14, 2022

Add Content switcher and Page menu buttons #568

Closed

7 tasks

obulat force-pushed the add/filter_sidebar branch 2 times, most recently from fb23e7b to ef989f4 Compare January 15, 2022 04:32

obulat force-pushed the add/content_switcher branch from 0d967c1 to 910d7f0 Compare January 15, 2022 06:49

obulat mentioned this pull request Jan 17, 2022

Add desktop content switcher to the home page #613

Merged

7 tasks

Base automatically changed from add/filter_sidebar to main January 17, 2022 14:55

krysal reviewed Jan 17, 2022

View reviewed changes

obulat added 7 commits January 17, 2022 21:30

Open filters in sidebar or modal from the header

3addc38

Add layout changes from add/content_switcher

ff733a3

Add content switcher and internal page menu

ada9e4f

Simplify header menu component

811ab0d

Remove NuxtLink from content type popover

2e05c5b

Make ContentSwitcher more flexible

3f38014

Set active content type using prop

f701e65

obulat force-pushed the add/content_switcher branch from 910d7f0 to f701e65 Compare January 17, 2022 18:34

Reuse components

3407175

obulat force-pushed the add/content_switcher branch from 827b20c to 3407175 Compare January 17, 2022 19:52

obulat added 3 commits January 17, 2022 22:57

Fix close button style

14e7798

Small fixes to imports

05c8554

Remove unused props

32d14ca

sarayourfriend suggested changes Jan 18, 2022

View reviewed changes

obulat and others added 2 commits January 18, 2022 18:35

Update src/components/VContentSwitcher/VContentTypes.vue

ad4a11c

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

Fix query comparisons

7ccb5f9

obulat requested a review from sarayourfriend January 18, 2022 15:55

sarayourfriend approved these changes Jan 18, 2022

View reviewed changes

krysal approved these changes Jan 18, 2022

View reviewed changes

obulat added 2 commits January 18, 2022 20:32

Fix mobile content switcher button not changing

31d5ad6

Merge branch 'main' into add/content_switcher

9e61478

# Conflicts: # src/locales/scripts/wp-locales.json

obulat merged commit be9dcf6 into main Jan 18, 2022

obulat deleted the add/content_switcher branch January 18, 2022 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Content Switcher component #607

Add Content Switcher component #607

obulat commented Jan 14, 2022 •

edited

Loading

obulat commented Jan 14, 2022 •

edited

Loading

krysal left a comment

krysal Jan 17, 2022

obulat Jan 17, 2022

sarayourfriend left a comment

sarayourfriend Jan 18, 2022

sarayourfriend Jan 18, 2022

sarayourfriend Jan 18, 2022

sarayourfriend Jan 18, 2022

obulat Jan 18, 2022

sarayourfriend Jan 18, 2022

sarayourfriend left a comment

krysal left a comment

Add Content Switcher component #607

Add Content Switcher component #607

Conversation

obulat commented Jan 14, 2022 • edited Loading

Fixes

Description

Issues

Testing Instructions

Checklist

Developer Certificate of Origin

obulat commented Jan 14, 2022 • edited Loading

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

sarayourfriend 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

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

obulat commented Jan 14, 2022 •

edited

Loading

obulat commented Jan 14, 2022 •

edited

Loading