Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1515: Add keywords field to the edit image description slide panel #1605

Conversation

joweecaquicla
Copy link

Description (*)

This PR will introduce the keywords field on the edit image details panel.

Fixed Issues (if relevant)

  1. Fixes magento/adobe-stock-integration# Add keywords field to the edit image description slide panel #1515: Add keywords field to the edit image description slide panel
  2. ...

Manual testing scenarios (*)

  1. ...
  2. ...

…panel - initial implementation of ui-select for keywords field
@joweecaquicla joweecaquicla linked an issue Jul 16, 2020 that may be closed by this pull request
@@ -48,6 +50,7 @@ define([
getDetails(this.imageEditDetailsUrl, [imageId]).then(function (imageDetails) {
this.images[imageId] = imageDetails[imageId];
this.image(this.images[imageId]);
this.mediaGalleryEditKeywordsDetails().getKeywordsOp(this.image().tags);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of setting keyword options - let's export the image from the image-edit component to the keywords-ui-select component

"#media-gallery-image-edit-keywords-details": {
"Magento_Ui/js/core/app": {
"components": {
"mediaGalleryEditKeywordsDetails": {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to declare and initialize this component as a child to the edit details component. Please see AdobeStockImageAdminUi/view/adminhtml/web/js/components/grid/column/image-preview.js - viewConfig for example

return Select.extend({
defaults: {
template: 'Magento_MediaGalleryUi/image/edit/keyword-ui-select',
options: []
Copy link
Member

Choose a reason for hiding this comment

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

options property is already declared and observed in the inherited component

joweecaquicla added 2 commits July 18, 2020 04:06
…panel - added keyword-ui-select as child component of image-edit, export image to keyword-ui-select, and modified getkeywordsop logic
Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @joweecaquicla ! See my comments

viewConfig: [
{
component: 'Magento_MediaGalleryUi/js/image/edit/keyword-ui-select',
name: '${ $.name }_select'
Copy link
Member

Choose a reason for hiding this comment

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

I'd use a more specific name for the keywords field/select

Suggested change
name: '${ $.name }_select'
name: '${ $.name }_keywords'

@@ -49,6 +82,7 @@ define([
this.images[imageId] = imageDetails[imageId];
this.image(this.images[imageId]);
this.openEditImageDetailsModal();
this.getKeywordsOp();
Copy link
Member

Choose a reason for hiding this comment

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

To avoid this call, simply make sure the keywords component is rendering options based on the imported image

@sivaschenko
Copy link
Member

@magento run all tests

@m2-community-project m2-community-project bot added Priority: P1 Needs to be fixed before any other issues Severity: S1 Affects critical data or functionality and forces users to employ a workaround labels Jul 29, 2020
@sivaschenko
Copy link
Member

@magento run all tests

@sivaschenko sivaschenko changed the title [WIP] #1515: Add keywords field to the edit image description slide panel #1515: Add keywords field to the edit image description slide panel Jul 29, 2020
Copy link
Contributor

@gabrieldagama gabrieldagama left a comment

Choose a reason for hiding this comment

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

HI @joweecaquicla @sivaschenko, thanks for the PR, please follow below some comments.

Comment on lines 33 to 43
viewConfig: [
{
component: 'Magento_Ui/js/form/element/ui-select',
name: '${ $.name }_keywords'
}
],
exports: {
keywordOptions: '${ $.name }_keywords:options'
},
links: {
selectedKeywords: '${ $.name }_keywords:value'
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should create a separate component like ui-keywords that extends ui-select, the way it is at the moment we have image-edit component changing linked properties and as consequence changing directly the ui-select state. By having separate component the component would have its own template and its own methods changing its own properties. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That's not actually keywords ui-select responsibility to change it's options/value. We may introduce a separate component for add new keyword field and button. However, in this case image-edit will still needs to be a proxy between ui-select and add-keyword components, so in my opinion that might increase the complexity. I'd postpone extracting additional components from image-edit

Copy link
Member

@sivaschenko sivaschenko Jul 30, 2020

Choose a reason for hiding this comment

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

As for adding the add keyword field to ui-select component approach - I wouldn't like to rewrite the ui-select component and template as that is a potential for conflicts on upgrades

*/
getOptionForKeyword: function(keyword) {
return {
'is_active': 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'is_active': 1,
is_active: 1,

@sivaschenko
Copy link
Member

@magento run all tests

@sivaschenko
Copy link
Member

Extracted task to use form ui component #1690

@sivaschenko
Copy link
Member

@magento run all tests

sivaschenko
sivaschenko previously approved these changes Aug 2, 2020
@sivaschenko
Copy link
Member

@magento run all tests

@sivaschenko
Copy link
Member

@magento run all tests

@sivaschenko
Copy link
Member

@magento run all tests

@sivaschenko sivaschenko merged commit fa4c4f2 into magento:2.0-develop Aug 2, 2020
@ghost
Copy link

ghost commented Aug 2, 2020

Hi @joweecaquicla, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P1 Needs to be fixed before any other issues Progress: done Severity: S1 Affects critical data or functionality and forces users to employ a workaround
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add keywords field to the edit image description slide panel
3 participants