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

Conversation

HILAYTRIVEDI
Copy link

@HILAYTRIVEDI HILAYTRIVEDI commented Mar 30, 2022

Fixes

Fixes #1197 by @zackkrida

Description

This PR will replace all the instances of the meta-search with additional sources available in the GitHub files.

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][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.

@HILAYTRIVEDI HILAYTRIVEDI requested a review from a team as a code owner March 30, 2022 05:41
@dhruvkb dhruvkb added this to Needs review in Openverse PRs Mar 30, 2022
@dhruvkb dhruvkb added the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Mar 30, 2022
Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

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

I have some preliminary feedback before diving into the full review. This is good and covers most of the usages. There are a few issues, however:

  • package-lock.json should not be modified, please revert it to the same version as on main.
  • Since "Additional sources" is not the name of a feature as "Meta search" was, it's grammatically incorrect in some places. But those wordings will need to be changed anyway as a part of the Meta search redesign Update the meta search section with the new design #734 so it's not a blocking change.

This PR will conflict with both #1190 and #1203. I would highly recommend waiting for those to get merged before working on this.

link: app.localePath('/meta-search'),
id: 'additional-sources',
name: 'header.additional-sources-nav-item',
link: app.localePath('/additional-sources'),
Copy link
Member

Choose a reason for hiding this comment

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

Since you've changed the link, you will need to rename src/pages/meta-search.vue to src/pages/additional-sources.vue. Nuxt uses file names to map to URLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to map a redirect for the original URL?

"intro": "{openverse} is built on top of a catalog that indexes CC-licensed and public domain content from selected sources. Learn more about our {link}.",
"link": "sources",
"license": "The internet is a big place! There are plenty of search portals and collections, hosting CC-licensed and public domain content, which we aren’t able to index or surface results from within the {openverse} interface. These are important sources of CC-licensed and public domain content, and we want to make sure that you are able to find the best openly licensed materials possible, regardless of where they are located.",
"content": "What content types can I search for using the Meta Search feature?",
"content-types": "Meta Search on {openverse} currently supports image, audio, and video search on external sites.",
"content": "What content types can I search for using the Additional Sources feature?",
Copy link
Member

@dhruvkb dhruvkb Mar 30, 2022

Choose a reason for hiding this comment

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

Example of a grammatical error introduced because "Additional Sources" is not the proper-noun of a feature name but rather an adjective-qualified common-noun.

Suggested change
"content": "What content types can I search for using the Additional Sources feature?",
"content": "What content types are supported by additional sources?",

Similar changes are needed in other places in en.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit here, but this isn't technically a grammatical error, and in fact could maybe even be clearer than what you suggested in context. At least in this case the phrase 'Additional Sources feature' groups it as a single block. In other strings you're right that it reads awkwardly to just replace "Meta Search" directly with "Additional Sources", but only because it's not qualifying "feature".

In general I think the wording is probably bad here because we're treating "Additional Sources" (even if lower cased) as a feature and also the name of a collection of external resources. I think it could be a good idea to revisit this group of strings altogether instead of doing a 1-to-1 term replacement. For example, this particular string is really confusing to read as it could be asking either "what content types do the websites listed in the additional sources feature support" or it could read "what content types does Openverse integrate 'additional sources' into"... both of which expose what I think is still a really unclear name we've chosen in "additional sources" (it's maybe even more unclear than the very obscure "meta search" if only because "meta search" is almost semantically meaningless the way we used it but "additional sources" is rife with ways of misunderstanding/misinterpreting).

@dhruvkb dhruvkb added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 📄 aspect: text Concerns the textual material in the repository and removed 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Mar 30, 2022
@HILAYTRIVEDI
Copy link
Author

Hi @dhruvkb, Thank you for the info. I will wait till the PRs will merge

Copy link
Author

@HILAYTRIVEDI HILAYTRIVEDI left a comment

Choose a reason for hiding this comment

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

I did the required changes But waiting for the PRs to be merged. once they are being pushed I will push my code too.

@dhruvkb
Copy link
Member

dhruvkb commented Mar 30, 2022

Thanks @HILAYTRIVEDI! Feel free to pick up another issue in the meantime.

@sarayourfriend
Copy link
Contributor

@HILAYTRIVEDI please note that we use pnpm, not npm to install dependencies. While things may work with npm install you won't be running with the exact versions of packages that we've locked to and tested with. Please do follow the instructions in the README's "Local Development" section to correctly set up your local environment (and avoid introducing things like package-lock.json for example).

Thanks for the contribution!

Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

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

@HILAYTRIVEDI are you still interested in completing this PR? Try resolving conflicts with main in that case, we can help if you have questions, or let us know if you can't continue working on this. Thanks!

@HILAYTRIVEDI
Copy link
Author

Hi @krysal , @dhruvkb asked me to hold this ticket untill 1190 and 1203 will merge. I come to find that they are merge now. So yes I will start to work on this now.

…l-sources

# Conflicts:
#	src/components/VMetaSearch/VMetaSearchForm.vue
#	src/locales/po-files/openverse.pot
#	src/pages/meta-search.vue
#	src/utils/get-legacy-source-url.js
Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this issue, @HILAYTRIVEDI !
I've merged main into your PR, and fixed the 'additional sources' page by renaming it to additional-sources.vue. It's still necessary to proofread the text of that page.

@zackkrida
Copy link
Member

Hi @HILAYTRIVEDI we have the final language this PR needs. You can see it here: #1197 (comment)

Would you like to continue working on this or have someone else take over?

@zackkrida
Copy link
Member

Closing in favor of #1665 due to some inactivity and needed changes.

@zackkrida zackkrida closed this Aug 15, 2022
Openverse PRs automation moved this from Needs review to Closed Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📄 aspect: text Concerns the textual material in the repository 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon
Projects
No open projects
Openverse PRs
  
Closed
Development

Successfully merging this pull request may close these issues.

Rename "Meta Search" to "External Sources"
6 participants