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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
92f8ef4
Convert search type buttons in the header to links
obulat Mar 17, 2022
710e40c
Temporarily replace the method that is only available in refactored s…
obulat Mar 18, 2022
4e53013
Add pot file
obulat Mar 18, 2022
e3a3f8c
Merge branch 'main' into use_links_for_header_search_type_switcher
obulat Mar 21, 2022
5a07f7c
Apply suggestions from code review
obulat Mar 22, 2022
158a8db
Apply changes from code review
obulat Mar 22, 2022
8ef03b3
Merge branch 'main' into use_links_for_header_search_type_switcher
obulat Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions src/components/VContentSwitcher/VSearchTypeItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<VItem
:selected="selected"
:is-first="itemId === 0"
v-bind="component"
@click.native="$emit('click', item)"
>
<div class="flex flex-row items-center text-base">
Expand All @@ -15,10 +16,10 @@
</template>

<script>
import { computed } from '@nuxtjs/composition-api'
import { computed, useContext, useRoute } from '@nuxtjs/composition-api'
import { defineComponent } from '@vue/composition-api'

import { contentStatus } from '~/constants/media'
import { ALL_MEDIA, contentStatus } from '~/constants/media'

import VIcon from '~/components/VIcon/VIcon.vue'
import VItem from '~/components/VItemGroup/VItem.vue'
Expand All @@ -30,16 +31,39 @@ 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.

obulat marked this conversation as resolved.
Show resolved Hide resolved
}
export default defineComponent({
name: 'VSearchTypeItem',
components: { VIcon, VItem, VPill },
props: propTypes,
setup(props) {
const { app } = useContext()
const route = useRoute()

const status = computed(() => {
return contentStatus[props.item]
})

/**
* The query is temporarily set to the incorrect value here: it uses the existing query
* that is set for selected search type. When the search store conversion PR is merged,
* we will set the query specific for the search type using `computeQueryParams(props.item)` method.
*/
const component = computed(() => {
if (props.isInHeader) {
return {
as: 'VLink',
href: app.localePath({
path: `/search/${props.item === ALL_MEDIA ? '' : props.item}`,
// query: searchStore.computeQueryParams(props.item),
query: route.value.query,
}),
}
} else return {}
obulat marked this conversation as resolved.
Show resolved Hide resolved
})
return {
component,
status,
}
},
Expand Down
1 change: 1 addition & 0 deletions src/components/VContentSwitcher/VSearchTypePopover.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
id="content-switcher-popover"
size="medium"
:active-item="activeItem"
:placement="placement"
@select="selectItem"
/>
</VPopover>
Expand Down
5 changes: 5 additions & 0 deletions src/components/VContentSwitcher/VSearchTypes.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
:item="item"
:item-id="idx"
:icon="content.icons[item]"
:is-in-header="placement === 'header'"
:selected="item === activeItem"
@click="handleClick(item)"
/>
Expand Down Expand Up @@ -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.

type: String,
required: true,
},
},
setup(props, { emit }) {
const content = useSearchType()
Expand Down
4 changes: 2 additions & 2 deletions src/locales/po-files/openverse.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ msgid ""
msgstr ""
"Project-Id-Version: Openverse \n"
"Report-Msgid-Bugs-To: https://github.com/wordpress/openverse/issues \n"
"POT-Creation-Date: 2022-03-18T03:49:09+00:00\n"
"POT-Creation-Date: 2022-03-21T07:54:31+00:00\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
Expand Down Expand Up @@ -49,7 +49,7 @@ msgctxt "search-type.heading"
msgid "Content types"
msgstr ""

#: src/components/VContentSwitcher/VSearchTypeItem.vue:11
#: src/components/VContentSwitcher/VSearchTypeItem.vue:12
msgctxt "search-type.status-beta"
msgid "Beta"
msgstr ""
Expand Down
11 changes: 5 additions & 6 deletions test/e2e/filters.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,12 @@ test('common filters are retained when media type changes from single type to al

await changeContentType(page, 'All content')

await expect(page).toHaveURL(
'/search/?q=cat&license_type=commercial&license=cc0&searchBy=creator'
)

for (let checkbox of ['cc0', 'commercial', 'creator']) {
await assertCheckboxStatus(page, checkbox)
}
await expect(page).toHaveURL(
'/search/?q=cat&license_type=commercial&license=cc0&searchBy=creator'
)
})

test('selecting some filters can disable dependent filters', async ({
Expand Down Expand Up @@ -138,12 +137,12 @@ test('filters are updated when media type changes', async ({ page }) => {

await changeContentType(page, 'Audio')

await expect(page).toHaveURL('/search/audio?q=cat&license=cc0')

await expect(page.locator('label:has-text("Tall")')).toHaveCount(0, {
timeout: 300,
})
await assertCheckboxStatus(page, 'cc0')

await expect(page).toHaveURL('/search/audio?q=cat&license=cc0')
})

test('new media request is sent when a filter is selected', async ({
Expand Down
6 changes: 4 additions & 2 deletions test/e2e/search-types.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,12 @@ for (let searchTypeName of ['audio', 'image']) {
}) => {
await page.goto('/search/?q=birds')
const contentLink = await page.locator(
`a[href*="/search/${searchTypeName}"][href$="q=birds"]`
`a:not([role="radio"])[href*="/search/${searchTypeName}"][href$="q=birds"]`
)
await expect(contentLink).toContainText(searchType.results)
await page.click(`a[href*="/search/${searchTypeName}"][href$="q=birds"]`)
await page.click(
`a:not([role="radio"])[href*="/search/${searchTypeName}"][href$="q=birds"]`
)

await expect(page).toHaveURL(searchType.url)
await checkSearchResult(page, searchType)
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const changeContentType = async (page, to) => {
await page.click(
`button[aria-controls="content-switcher-popover"], button[aria-controls="content-switcher-modal"]`
)
await page.click(`button[role="radio"]:has-text("${to}")`)
await page.click(`a:has-text("${to}")`)
}
/**
* Finds a button with a popup to the left of the filters button which doesn't have a 'menu' label
Expand Down