Skip to content

Conversation

@valentinboyanov
Copy link
Contributor

Description (*)

Add Magento admin locale to search API requests so admin user can see localized results.

Fixed Issues (if relevant)

  1. Admin user can search images by keywords #7 Admin user can search images by keywords

Manual testing scenarios (*)

  1. Change locale configuration for 'Default config': Stores > Configuration > General > Locale options > Locale
  2. Search stock images by keywords in admin (UI Component)

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.

Hi @valentinboyanov , thanks for the pull request!

To be honest, I think that Client class is too low level and too specific to interact with a locale resolver.

The locale should be retrieved from the RequestInterface.

Could you update the changes and set the locale in \Magento\AdobeStockImage\Model\GetImageList::execute to request builder and then retrieve it frorm the the RequestInterface in Client

);
}

private function locale(): string
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use a private method instead of direct usage of the dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First I extracted the locale value to a method in order to express the concept. And then I chose the LocaleResolver to provide the expected value. But now I think that changing the name of the collaborator I will achieve the same avoiding one level of indirection :)

@sidolov sidolov merged commit 01f59ef into magento:develop May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants