[Inserter]: Openverse integration #46251
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Feature] Inserter
ntsekouras
requested review from
youknowriad,
mtias,
jameskoster,
jasmussen and
zackkrida
|
Size Change: +3.5 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Nice, took it for a spin, here's the inserter > media > OV flow in gif form:
I think the focus style when arrowing through items is the stock browser one. We can probably use the native GB one, curious why it doesn't get it automatically. Should images inserted from Openverse default to having a caption with accreditation? Perhaps a question for @zackkrida |
ntsekouras
force-pushed
the
try/integrate-openverse
branch
4 times, most recently
from
36925f6
to
cea1218
Compare
ntsekouras
requested review from
spacedmonkey,
ellatrix and
nerrad
as code owners
I think that's actually a good thing personally, what we could have here is an openverse package where the "fetch" function of this specific category is defined (or a part of it) but it's definitely ok to consider that package later more thoughtfully. |
ntsekouras
changed the title
If it's possible to do so, I think the rich text/HTML that the Openverse frontend detail view provides is a great format:
|
|
@ntsekouras we also have some TypeScript code in our frontend repo which shows how we build up attribution HTML: I'll get you a full review of the PR later this week. |
ntsekouras
force-pushed
the
try/integrate-openverse
branch
from
d397875
to
c61f1fb
Compare
ntsekouras
commented
Dec 15, 2022
ntsekouras
commented
Dec 15, 2022
ntsekouras
force-pushed
the
try/integrate-openverse
branch
from
df34009
to
a393ce8
Compare
ellatrix
approved these changes
Dec 16, 2022
There was a problem hiding this comment.
Looks good once the translation strings are fixed. I guess this still needs a check from someone from Openverse.
Minor: I'm not a big fan of the code duplication, but we discussed it and it seems a fine temporary solution. Would be nice to move it to a new media inserter or openverse package at some point.
packages/block-editor/src/components/inserter/media-tab/media-list.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/block-editor/inserter-media-categories.js
Outdated
Show resolved
Hide resolved
|
Update Right now the PR integrates Openverse and changes the
The remaining task I believe can be done iteratively, which are part of the PR's description(pagination, report link, extra space at the bottom of the media list). What do you think? |
ntsekouras
force-pushed
the
try/integrate-openverse
branch
from
7c5bf13
to
fee64f2
Compare
|
This isn't a blocker by any means in my opinion, but I'm curious if we considered adding an Openverse icon after the name of the service. For the Jetpack integration there are images for each service included in the list. If in the future we have additional services that are included, people can recognize them visually. Example from Jetpack integration: |
That could be tracked in a separate issue and be tackled as we augment the API with other things as well. |
|
Thanks, I can created an issue for this. |
mcsf
reviewed
Jan 20, 2023
| license_version: licenseVersion, | ||
| license_url: licenseUrl, | ||
| } = item; | ||
| /* translators: Openverse default media item title in the block inserter's media list. (ex. 'This work by {creator} is marked with CC0 1.0'). */ |
There was a problem hiding this comment.
This function should revolve around just two possible strings:
'This work by %1$s is marked with %2$s''"%1$s" by %2$s is marked with %3$s'
We can't be stitching things up like "${ _title }" ${ creator }/ ...
packages/block-editor/src/components/inserter/media-tab/utils.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inserter/media-tab/hooks.js
Outdated
Show resolved
Hide resolved
| const baseCssClass = 'block-editor-inserter__media-list'; | ||
| let truncatedTitle; | ||
| if ( title.length > MAXIMUM_TITLE_LENGTH ) { | ||
| const omission = '...'; |
There was a problem hiding this comment.
Not urgent, but wondering if this should be __( '...' ) or in some other way sourced from the current locale.
There was a problem hiding this comment.
Not sure about this one.. I used the same approach with useBlockDisplayTitle hook, so I guess we be consistent with whatever we decide..
Co-authored-by: Miguel Fonseca <[email protected]>
* Remove quotes around the word `Work` when a work has no title. * To make strings more obvious to translators when a work has no title, hardcode the link in the string. For this to work, extract the function `getExternalLinkAttributes`. * Instead `__()`, use `_x()` with the context `'caption'`. * Fix typos and clarify `translators:` notes. * Add a space before the slash in between creator and license.
mcsf
approved these changes
Jan 23, 2023
This was referenced Jan 23, 2023
This was referenced Mar 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves: #46222
This PR integrates Openverse through inserter's
Mediatab.In order to do that and having in mind how to make media categories easily extensible, I have made some implementation changes. I have replaced
__unstableFetchMediawith__unstableInserterMediaCategories. Each media category is responsible to implement their ownfetchfunction and this is important because some external sources might have different requirements(ex. authentication), default and supported arguments. In order for this to work well I have added two interfaces(InserterMediaRequest andInserterMediaItem`) that each category should also use to map the fetch request and the returned media items.Notes
I'm not sure yet in which package these categories should be added. I don't think they fit in
core-data, so I've added them ineditorandedit-site. The drawback here is that we have to keep them in sync.Tasks
caption? Should we construct something similar to Openverse text version(ex `"Riga" by Ullisan is licensed under CC BY-ND 2.0. To view a copy of this license, visit https://creativecommons.org/licenses/by-nd/2.0/?ref=openverse.)?Add a disclaimer message for the search results from Openverse([Inserter]: Add media tab #44918 (comment)). Should we add a disclaimer in the form of a media category description like we do in Patterns in the inserter?Since we have excluded some sources where a disclaimer message would be good to have, we can add this when needed. This would also give us some time to refine the appearance and wording of the text.
report icon(that probably opens a modal with confirmation textDo you want to report this? You can do it in the external sourceor something like that and provide a link. Can this be done in a follow up? 🤔open this search in Openverse(can be done in a follow up).load morebuttons is not good for a11y and we would need something likeprevious/next. This affects all media categories. I believe this could be done in a follow up.Screenshots or screencast
Screen.Recording.2022-12-08.at.11.23.13.AM.mov