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

IBX-7895: Added possibility to change create content button label #1200

Draft
wants to merge 4 commits into
base: 4.6
Choose a base branch
from

Conversation

Gengar-i
Copy link
Contributor

@Gengar-i Gengar-i commented Mar 5, 2024

Question Answer
JIRA issue IBX-7895
Type Improvement
Target version v4.6
BC breaks no
Doc needed no
Required https://github.com/ibexa/dashboard/pull/115

Added possibility to change create content button label

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Asked for a review (ping @ibexa/engineering).

@Gengar-i Gengar-i marked this pull request as draft March 5, 2024 17:28
use Ibexa\AdminUi\Specification\AbstractSpecification;
use Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType;

class ContentTypeIsDashboardContainer extends AbstractSpecification
Copy link
Member

Choose a reason for hiding this comment

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

ibexa/admin-ui must not have any knowledge about the ibexa/dashboard

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not resolved, you are still checking for dashboard.container_content_type_identifier parameter. What you need here is an extension point and contain that whole logic inside dashboard package.

@Gengar-i Gengar-i requested a review from adamwojs March 6, 2024 10:01
Copy link

sonarcloud bot commented Mar 6, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

public function createLabel(ContentType $contentType): string;
}

class_alias(ContentRightSidebarLabelFactoryInterface::class, 'EzSystems\EzPlatformAdminUi\Menu\ContentRightSidebarLabelFactoryInterface');
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are not needed in new code.

public const CREATE_USER = 'sidebar_right.create_user';

/** @var \Ibexa\Contracts\Core\SiteAccess\ConfigResolverInterface */
private $configResolver;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private $configResolver;
private ConfigResolverInterface $configResolver;

@@ -10,6 +10,12 @@ services:

Ibexa\AdminUi\Menu\MenuItemFactory: ~

#
# Right Sidebar Label Factory
#
Copy link
Contributor

Choose a reason for hiding this comment

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

From my pov, this comment adds no value.

Comment on lines +32 to +40
/**
* Returns label based on content type.
*
* @param \Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType $contentType
*
* @return string
*
* @throws \Ibexa\AdminUi\Exception\InvalidArgumentException
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* Returns label based on content type.
*
* @param \Ibexa\Contracts\Core\Repository\Values\ContentType\ContentType $contentType
*
* @return string
*
* @throws \Ibexa\AdminUi\Exception\InvalidArgumentException
*/
/**
* @throws \Ibexa\AdminUi\Exception\InvalidArgumentException
*/

@Gengar-i Gengar-i changed the base branch from main to 4.6 March 15, 2024 09:05
@mikadamczyk
Copy link
Contributor

@dew326 Will this pull request be taken over by someone from the team?

@dew326
Copy link
Contributor

dew326 commented Jul 10, 2024

@mikadamczyk It is a pure php solution, so we should rather look for some backend developer if we want to make progress on that.

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.

7 participants