Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

EZP-32164: Implemented IconPathResolver service #24

Merged
merged 5 commits into from
Nov 18, 2020
Merged

EZP-32164: Implemented IconPathResolver service #24

merged 5 commits into from
Nov 18, 2020

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Nov 17, 2020 •
edited
Loading

Question Answer
JIRA issue EZP-32164
Improvement no
New feature yes
Target version 2.2
BC breaks no
Tests pass yes
Doc needed no

TODO:

  • Implement feature / fix a bug.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@webhdx webhdx added the Improvement Good for newcomers label Nov 17, 2020
@webhdx webhdx self-assigned this Nov 17, 2020
@webhdx webhdx changed the base branch from master to 1.0 November 17, 2020 12:54
@webhdx webhdx changed the base branch from 1.0 to 2.2 November 17, 2020 12:55
@webhdx webhdx force-pushed the icon_path_resolver branch from 5f1728a to 6570034 Compare November 17, 2020 13:00
@webhdx webhdx force-pushed the icon_path_resolver branch from 6570034 to 51614da Compare November 17, 2020 13:04
@webhdx webhdx requested review from lserwatka, micszo and a team November 17, 2020 13:56
alongosz
alongosz requested changes Nov 17, 2020
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It should be quite quick to add simple unit test coverage for the implementation, just one method (with and without 2nd argument). Otherwise looks good.

CS remark:

src/IbexaPlatformAssetsBundle/lib/Resolver/IconPathResolverInterface.php Outdated

interface IconPathResolverInterface
{
public function resolve(string $icon, string $set = null): string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function resolve(string $icon, string $set = null): string;
public function resolve(string $icon, ?string $set = null): string;

@webhdx
Copy link
Contributor Author

webhdx commented Nov 17, 2020 •
edited
Loading

@alongosz Always feels great to remind yourself about your good ol' friend unit tests: d2efa7b

image

@webhdx
Copy link
Contributor Author

webhdx commented Nov 17, 2020

Travis should be green in a minute

alongosz
alongosz approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Thx, I know it was a painful experience! ;) Looks good.
Minor CS:

tests/IbexaPlatformAssetsBundle/lib/Resolver/IconPathResolverTest.php Outdated Show resolved Hide resolved
@webhdx
EZP-32164: Added missing return type for
557d922 
IconPathResolverTest::resolveDataProvider
@lserwatka lserwatka merged commit c24f3b9 into 2.2 Nov 18, 2020
@lserwatka lserwatka deleted the icon_path_resolver branch November 18, 2020 10:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bogusez bogusez bogusez approved these changes

@alongosz alongosz alongosz approved these changes

@lserwatka lserwatka Awaiting requested review from lserwatka

@micszo micszo Awaiting requested review from micszo

Assignees

@webhdx webhdx

Labels
Improvement Good for newcomers QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@webhdx @alongosz @bogusez @lserwatka