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

IBX-902: Refactored GroupedFields as a separate service #46

Merged
merged 4 commits into from
Sep 13, 2021
Merged

IBX-902: Refactored GroupedFields as a separate service #46

merged 4 commits into from
Sep 13, 2021

Conversation

damianz5
Copy link
Contributor

@damianz5 damianz5 commented Sep 3, 2021 •
edited
Loading

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-902
Bug fix? no
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Refactored GroupedFields as a separate service.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@damianz5 damianz5 force-pushed the IBX-902-cotf-field-tabs branch 5 times, most recently from 3aaef07 to e3adb48 Compare September 6, 2021 09:17
@damianz5 damianz5 changed the title IBX-902: When creating content on the fly doesn't have categories tabs IBX-902: Refactored GroupedFields as a separate service Sep 6, 2021
@damianz5 damianz5 marked this pull request as ready for review September 6, 2021 09:18
@damianz5 damianz5 mentioned this pull request Sep 6, 2021
Merged
2 tasks
Steveb-p
Steveb-p requested changes Sep 6, 2021
View reviewed changes
composer.json Outdated Show resolved Hide resolved
src/contracts/Content/Form/Provider/GroupedContentFormFieldsProviderInterface.php Outdated Show resolved Hide resolved
src/lib/Content/Form/Provider/GroupedContentFormFieldsProvider.php Outdated Show resolved Hide resolved
src/lib/Content/Form/Provider/GroupedContentFormFieldsProvider.php Outdated Show resolved Hide resolved
src/lib/Content/View/Builder/ContentEditViewBuilder.php
Comment on lines +120 to +122
'grouped_fields' => $this->groupedContentFormFieldsProvider->getGroupedFields(
$form->get('fieldsData')->all()
),
Copy link

Choose a reason for hiding this comment

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

TBH while it's a change to initial implementation, I believe that probably AbstractType::buildView() from the Form component should have been used instead for grouping data for the form. After all, how fields are grouped should be part of the form itself, not part of a view.

Just a note though. Optional.

tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php Outdated Show resolved Hide resolved
tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php Outdated Show resolved Hide resolved
tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php Outdated Show resolved Hide resolved
tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php Show resolved Hide resolved
tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php Outdated Show resolved Hide resolved
@damianz5 damianz5 force-pushed the IBX-902-cotf-field-tabs branch from 370f667 to a694752 Compare September 6, 2021 10:51
@damianz5 damianz5 requested review from Steveb-p and a team September 6, 2021 13:56
@damianz5 damianz5 force-pushed the IBX-902-cotf-field-tabs branch from d2f50a1 to 0d10794 Compare September 7, 2021 08:55
@damianz5 damianz5 requested a review from Steveb-p September 7, 2021 09:08
Steveb-p
Steveb-p approved these changes Sep 7, 2021
View reviewed changes
Copy link

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

One final nitpick, but otherwise 👍

tests/lib/Content/Form/Provider/GroupedContentFormFieldsProviderTest.php Outdated Show resolved Hide resolved
@damianz5 damianz5 force-pushed the IBX-902-cotf-field-tabs branch from 0d10794 to ad13bc9 Compare September 7, 2021 09:54
@mnocon mnocon mentioned this pull request Sep 8, 2021
Merged
@damianz5 damianz5 force-pushed the IBX-902-cotf-field-tabs branch from ad13bc9 to a4831a7 Compare September 8, 2021 10:25
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 8, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

tomaszszopinski
tomaszszopinski approved these changes Sep 10, 2021
View reviewed changes
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on Ibexa DXP 3.3.x-dev with diff.

@Nattfarinn Nattfarinn merged commit 9beed02 into 1.3 Sep 13, 2021
@Nattfarinn Nattfarinn deleted the IBX-902-cotf-field-tabs branch September 13, 2021 06:53
@Nattfarinn
Copy link
Contributor

@damianz5 Please merge up to master and do fast-forward merge to ibexa org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs approved these changes

@mikadamczyk mikadamczyk mikadamczyk approved these changes

@Steveb-p Steveb-p Steveb-p approved these changes

@alongosz alongosz alongosz approved these changes

@webhdx webhdx webhdx approved these changes

@tomaszszopinski tomaszszopinski tomaszszopinski approved these changes

Assignees
No one assigned
Labels
QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@damianz5 @Nattfarinn @adamwojs @mikadamczyk @Steveb-p @alongosz @webhdx @tomaszszopinski