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-3523: Introduced tabs to left sidebar in Content Edit interface #526

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Aug 3, 2022

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-3523
Complementary PR https://github.com/ibexa/taxonomy/pull/142
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? yes
License GPL-2.0

image

This PR changes sidebar menu in content editing interface to use tabs. Idea behind this is to separate typical fieldtypes from fieldtypes enriching content with new functionality. Good example is SEO - it isn't a field per se but rather a fieldtype handling functionality for the whole Content object. Taxonomy is another good example. Items assigned to Meta tabs are configured using semantic configuration:

ibexa:
    system:
        admin_group:
            admin_ui_forms:
                content_edit:
                    fieldtypes:
                        ibexa_taxonomy_entry_assignment: 	# fieldtype identifier
                            meta: true 						# put this fieldtype under Meta tab

Extending the menu

<?php

namespace App\EventListener;

use Ibexa\AdminUi\Menu\ContentEditAnchorMenuBuilder;
use Ibexa\AdminUi\Menu\Event\ConfigureMenuEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class MyMenuListener implements EventSubscriberInterface
{
    public static function getSubscribedEvents(): array
    {
        return [ConfigureMenuEvent::CONTENT_EDIT_ANCHOR_MENU => 'onAnchorMenuConfigure'];
    }

    public function onAnchorMenuConfigure(ConfigureMenuEvent $event): void
    {
        $menu = $event->getMenu(); // root
        
        $menu[ContentEditAnchorMenuBuilder::ITEM__CONTENT]; // Content tab
        
        $menu[ContentEditAnchorMenuBuilder::ITEM__META]; // Meta tab
        
        // add new tab
        $menu->addChild('item', ['attributes' => ['data-target-id' => 'ibexa-edit-content-sections-item']]); // data-target-id has to match with component
        
        // add 2nd level menu
        $menu['item']->addChild('item_2', ['attributes' => ['data-target-id' => 'ibexa-edit-content-sections-item-item_2']]); // data-target-id has to match with secondary section id's
    }
}

For new tabs it's also required to render its section in content editing form. This is done with UI Component:

    app.my_component:
        parent: Ibexa\AdminUi\Component\TwigComponent
        arguments:
            $template: 'my_component.html.twig'
        tags:
            - { name: ibexa.admin_ui.component, group: 'content-edit-sections' } # note the group name
# my_component.html.twig

{% extends '@ibexadesign/ui/component/anchor_navigation/primary_section.html.twig' %}

{% set data_id = 'ibexa-edit-content-sections-item' %}

{% block sections %}
    {% embed '@ibexadesign/ui/component/anchor_navigation/secondary_section.html.twig' %}
        {% set data_id = 'ibexa-edit-content-sections-item-item_2' %}
        {% block content %}
			Contents of 'item_2' secondary section
        {% endblock %}
    {% endembed %}
{% endblock %}

Deprecations

I've deprecated admin_ui_forms.content_edit_form_templates config setting in favor of making content_edit an array where we can add more settings related to content editing form (i.e. content_edit.fieldtypes as in this PR)

system:
    admin_group:
        admin_ui_forms:

            # deprecated setting:
            content_edit_form_templates: 
                    - { template: '@ibexadesign/content/form_fields.html.twig', priority: 0 }
            
            # use config below instead:
            content_edit:
                form_templates:
                    - { template: '@ibexadesign/content/form_fields.html.twig', priority: 0 }

Both notations will work until deprecation is removed in 5.0. Matching siteaccess aware parameters have not been renamed as it's more difficult to provide backwards compatibility on those.

Checklist:

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

@webhdx webhdx added Doc needed The changes require some documentation Feature New feature request Ready for review labels Aug 3, 2022
@webhdx webhdx requested review from a team August 3, 2022 13:31
@webhdx webhdx self-assigned this Aug 3, 2022
@webhdx webhdx mentioned this pull request Aug 3, 2022
2 tasks
@lserwatka
Copy link
Contributor

@webhdx you must fix SonarCloud issues too.

@GrabowskiM GrabowskiM self-assigned this Aug 4, 2022
contentContainer.style.paddingBottom = '0px';

if (!firstSection.isSameNode(lastSection) && lastSection && lastSection.offsetHeight) {
const headerContainer = doc.querySelector('.ibexa-edit-header .ibexa-edit-header__container');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this parent in selector? .ibexa-edit-header

items.forEach((item) => item.classList.toggle('ibexa-navigation-menu__secondary--active', item.isSameNode(node)));
};
const onSelectPrimaryMenuList = (event) => {
const { targetId } = event.target.dataset;
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
const { targetId } = event.target.dataset;
const { targetId } = event.currentTarget.dataset;

}
};
const onSelectPrimaryMenuDropdown = (event) => {
const targetId = event.target.value;
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
const targetId = event.target.value;
const targetId = event.currentTarget.value;

{% set sanitized_item = item|slug %}
<li class="ibexa-anchor-navigation-menu__item">
<button
type="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

data-id="ibexa-edit-content-sections-content-fields"
>
{% block form_fields %}
<div class="ibexa-anchor-navigation-sections">
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

@@ -0,0 +1,8 @@
<div
class="ibexa-edit-content__primary-section"
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

@@ -0,0 +1,6 @@
<div
data-id="{{ data_id }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

@webhdx webhdx requested a review from a team August 4, 2022 09:37
@alongosz
Copy link
Member

alongosz commented Aug 4, 2022

@webhdx nice description. If it lands in the Doc, I'd have one small nitpick for it - service name should follow our and SF guidelines, so rather app.my_component

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.

+1 for backend side, but other Reviewers had important remarks.

@alongosz alongosz requested a review from a team August 4, 2022 11:04
Copy link
Contributor

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

#526 (comment) was left behind, other than this 👍

@GrabowskiM GrabowskiM requested a review from dew326 August 5, 2022 08:41
@micszo micszo self-assigned this Aug 5, 2022
}
};

bindPrimaryMenuListEvents();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not attach<...> as we have in other places?

Comment on lines 19 to 20
const firstSection = primarySection.querySelector('.ibexa-edit-content__secondary-section:first-child');
const lastSection = primarySection.querySelector('.ibexa-edit-content__secondary-section:last-child');
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be confusing because :first-child and :last-child "ignore" the class. This will return null in case .ibexa-edit-content__secondary-section is not first/last element, but it is assumed below that firstSection will never be null, so:

Suggested change
const firstSection = primarySection.querySelector('.ibexa-edit-content__secondary-section:first-child');
const lastSection = primarySection.querySelector('.ibexa-edit-content__secondary-section:last-child');
const firstSection = primarySection.firstElementChild;
const lastSection = primarySection.lastElementChild;


contentContainer.style.paddingBottom = '0px';

if (!firstSection.isSameNode(lastSection) && lastSection && lastSection.offsetHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that lastSection will never be null, given firstSection is not null.

Suggested change
if (!firstSection.isSameNode(lastSection) && lastSection && lastSection.offsetHeight) {
if (!firstSection.isSameNode(lastSection) && lastSection.offsetHeight) {

const lastSection = primarySection.querySelector('.ibexa-edit-content__secondary-section:last-child');
const contentContainer = contentColumn.querySelector('.ibexa-edit-content__container');

contentContainer.style.paddingBottom = '0px';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to set it even for a case in which we are setting it below again?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's kinda of a reset, notice that we set this style after two ifs, in all other cases it should be set to 0

};
let currentlyVisibleSections = getSecondarySectionActiveItems();
const fitSecondarySections = () => {
const primarySection = doc.querySelector('.ibexa-edit-content__primary-section--active');
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes you are using formContainerNode and sometimes not, is there any reason behind it?


fitSecondarySections();
};
const showSecondaryMenu = (node) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have such a function also for the primary menu.

color: $ibexa-color-info;
font-weight: 600;

&:before {
Copy link
Contributor

Choose a reason for hiding this comment

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

here and in other places below:

Suggested change
&:before {
&::before {

@@ -51,6 +51,34 @@
</div>
{% endblock %}

{% block anchor_menu %}
{% set content_edit_anchor_menu = knp_menu_get('ibexa.admin_ui.menu.content_edit.anchor_menu', [], {
'content': content,
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick here and in other places:

Suggested change
'content': content,
content,

@micszo
Copy link
Contributor

micszo commented Aug 5, 2022

Small frontend issue: http://g.recordit.co/W2X0mTjQbM.gif

@micszo
Copy link
Contributor

micszo commented Aug 5, 2022

@GrabowskiM the second level menu item width still changes
also the new menu does not behave correctly when switching tabs
http://g.recordit.co/jcAeY6GdGq.gif

@sonarcloud
Copy link

sonarcloud bot commented Aug 8, 2022

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 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@micszo micszo 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 Commerce 4.2.x-dev.

@micszo micszo removed their assignment Aug 8, 2022
@webhdx webhdx merged commit 3fefb3e into main Aug 8, 2022
@webhdx webhdx deleted the IBX-3523_As_an_Editor_I_want_to_organize_fieldtypes_in_different_tabs_in_Content_Edit_sidebar branch August 8, 2022 12:01
@juskora juskora removed the Doc needed The changes require some documentation label Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature request QA approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.