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

Add tab to adminUI to display comments #34

Merged
merged 15 commits into from
Sep 24, 2019

Conversation

dspe
Copy link
Contributor

@dspe dspe commented Nov 10, 2018

First step to bring it back CommentsBundle into eZ Platform UI (v2 Admin UI)

@ezrobot

This comment has been minimized.

@andrerom andrerom requested review from alongosz and bdunogier January 7, 2019 09:12
Copy link
Member

@bdunogier bdunogier left a comment

Choose a reason for hiding this comment

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

Good changes, but the requirements need to be clarified. You have a service and a class that depend on admin-ui, since it extends EzSystems\EzPlatformAdminUi\Tab\AbstractTab. It means that this patch requires v2.

Maybe this could be kept compatible with v1 & v2 by only moving the tab service to a v2 dedicated file, and only load it when admin-ui is installed ?

@dspe
Copy link
Contributor Author

dspe commented Feb 20, 2019

Indeed @bdunogier . I will check maybe with the PM first to know if we keep BC or if we could break it to a new version .

@bdunogier
Copy link
Member

After discussing the matter with @dspe, we decided to go for v1 & v2 compatibility by only load the admin related services if ezplatform-admin-ui is installed.

@ezrobot

This comment has been minimized.

@ezrobot

This comment has been minimized.

@dspe
Copy link
Contributor Author

dspe commented Feb 25, 2019

Small update. maybe something better to do ? @bdunogier any feedbacks? and how to fix Travis too ?

.travis.yml Outdated Show resolved Hide resolved
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.

I missed that review request, better late than never. But... well... Guys... I'm very sorry... -1 on this approach from my side. This is anti-pattern.

I know you neither agree with nor like my perspective, so not going to request changes. Just making a comment that you're free to ignore:

DependencyInjection/EzSystemsCommentsExtension.php Outdated Show resolved Hide resolved
Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

👍

Aslo merged in style fixes and updates in master, so ready for last review now, so unless there is some nitpick new style rule failing on the new code Travis should be expected to be green now 📗

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.

Looks good.
Nitpicks:

Resources/translations/comments.en.xlf Outdated Show resolved Hide resolved
Resources/translations/comments.fr.xlf Outdated Show resolved Hide resolved
Tab/CommentsTab.php Show resolved Hide resolved
@andrerom andrerom merged commit 2bed443 into ezsystems:master Sep 24, 2019
@andrerom
Copy link
Contributor

andrerom commented Sep 24, 2019

Released v7.0.0-beta1 for those that want to test this.

@bdunogier / @SylvainGuittard Maybe you can schedule this being added in upcoming 2.5.x demo / ee-demo release => so QA has a way to test this => so it can reach stable?

@SylvainGuittard
Copy link

Thanks @andrerom
👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants