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.

Sign up for GitHub

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

Jump to bottom

Improve opendialogurl configuration to be extensible #27863

Closed
wants to merge 10 commits into from
Closed

Improve opendialogurl configuration to be extensible #27863

wants to merge 10 commits into from

Conversation

Nazar65
Copy link
Member

@Nazar65 Nazar65 commented Apr 15, 2020 •
edited by slavvka
Loading

Description (*)

Related Pull Requests

magento/magento2-page-builder#450
magento/adobe-stock-integration#1186

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title
    N/A

Manual testing scenarios (*)

Verify that media gallery opened corrects in all cases
1.with AdobeStockIntegration && PageBuilder

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Apr 15, 2020

Hi @Nazar65. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@Nazar65 Nazar65 mentioned this pull request Apr 15, 2020
Merged
@engcom-Golf engcom-Golf mentioned this pull request Apr 15, 2020
Closed
4 tasks
@Nazar65
Copy link
Member Author

Nazar65 commented Apr 15, 2020

Functional Test failing on Page Builder repo on dev branch, as pr linked with PB, they are failing too,
This failure not related to the changes in this pr

sivaschenko
sivaschenko requested changes Apr 15, 2020
View reviewed changes
app/code/Magento/Ui/Component/Form/Element/DataType/Media/OpenDialogUrl.php Outdated
/**
* @param string $openDialog
*/
public function __construct(string $openDialog = null)
Copy link
Member

Choose a reason for hiding this comment

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

The class name already contains OpenDialog

Suggested change
public function __construct(string $openDialog = null)
public function __construct(string $url = null)

app/code/Magento/Ui/Component/Form/Element/DataType/Media/OpenDialogUrl.php Outdated
/**
* Returns open dialog url for media browser
*/
public function getOpenDialogUrl(): string
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 duplicate naming between class and method

Suggested change
public function getOpenDialogUrl(): string
public function get(): string

app/code/Magento/Ui/Component/Form/Element/DataType/Media/Image.php Outdated
/**
* @var OpednDialogUrl
*/
private $openDialogConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep consistent naming between class, property and variable to avoid confusion

Suggested change
private $openDialogConfig;
private $openDialogUrl;

@ghost ghost assigned sivaschenko Apr 15, 2020
@engcom-Golf
Copy link
Contributor

@magento run all tests

@engcom-Golf
Copy link
Contributor

@magento run all tests

sivaschenko
sivaschenko requested changes Apr 15, 2020
View reviewed changes
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 update @Nazar65 !

Can we also use the OpenDialogUrl provider in:

  • \Magento\Cms\Model\Wysiwyg\Gallery\DefaultConfigProvider::getConfig
  • \Magento\Tinymce3\Model\Config\Gallery\Config::getConfig

@engcom-Golf
Copy link
Contributor

@magento run all tests

1 similar comment
@engcom-Golf
Copy link
Contributor

@magento run all tests

@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, thank you for the review.
ENGCOM-7433 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@sivaschenko sivaschenko added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Apr 15, 2020
@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, thank you for the review.
ENGCOM-7433 has been created to process this Pull Request

@VladimirZaets VladimirZaets added this to the 2.4.0 milestone Apr 15, 2020
@VladimirZaets VladimirZaets added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S1 Affects critical data or functionality and forces users to employ a workaround. labels Apr 15, 2020
@slavvka
Copy link
Member

slavvka commented Apr 15, 2020

@magento run all tests

@slavvka
Copy link
Member

slavvka commented Apr 16, 2020

@magento run all tests

slavvka
slavvka requested changes Apr 16, 2020
View reviewed changes
Copy link
Member

@slavvka slavvka left a comment

Choose a reason for hiding this comment

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

@Nazar65 Please fix the failing tests

@ghost ghost assigned slavvka Apr 16, 2020
@Nazar65
Copy link
Member Author

Nazar65 commented Apr 16, 2020

@magento run all tests

2 similar comments
@engcom-Golf
Copy link
Contributor

@magento run all tests

@engcom-Golf
Copy link
Contributor

@magento run all tests

@Nazar65
Copy link
Member Author

Nazar65 commented Apr 16, 2020

Closing as this changes moved to #27566

@Nazar65 Nazar65 closed this Apr 16, 2020
@m2-assistant
Copy link

m2-assistant bot commented Apr 16, 2020

Hi @Nazar65, 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
Reviewers

@slavvka slavvka slavvka requested changes

@sivaschenko sivaschenko sivaschenko approved these changes

Assignees

@sivaschenko sivaschenko

@slavvka slavvka

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: Cms Component: Tinymce Component: Ui Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: needs update Release Line: 2.4 Severity: S1 Affects critical data or functionality and forces users to employ a workaround.
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Nazar65 @engcom-Golf @magento-engcom-team @slavvka @sivaschenko @VladimirZaets