IBX-397: Implement the new design for content item editing screen #1770

lucasOsti · 2021-06-01T14:03:24Z

Question	Answer
Tickets	https://issues.ibexa.co/browse/IBX-397
Related to	ezsystems/ez-support-tools#97
	https://github.com/ezsystems/ezplatform-workflow/pull/193
	https://github.com/ezsystems/ezplatform-segmentation/pull/28
	https://github.com/ezsystems/ezplatform-site-factory/pull/124
	https://github.com/ezsystems/ezplatform-calendar/pull/75
	https://github.com/ezsystems/ezplatform-page-builder/pull/765
Bug fix?	no
New feature?	yes
BC breaks?	yes
Tests pass?	no
Doc needed?	??
License	GPL-2.0

Redesign content create and main layout

Checklist:

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

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

src/bundle/Resources/public/js/scripts/admin.content.edit.js

src/bundle/Resources/public/js/scripts/helpers/tooltips.helper.js

src/bundle/Resources/public/scss/_icons.scss

src/bundle/Resources/public/scss/_knp-menu.scss

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

GrabowskiM · 2021-06-07T13:21:34Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+    const navigateTo = (event) => {
+        const { anchorTargetSection } = event.currentTarget.dataset;
+
+        getSectionsCords();


Maybe put definition of this func before?

GrabowskiM · 2021-06-07T13:22:11Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+    };
+
+    doc.querySelectorAll('.ibexa-anchor-navigation-menu__btn').forEach((button) => {
+        button.addEventListener('click', navigateTo);


Suggested change

button.addEventListener('click', navigateTo);

button.addEventListener('click', navigateTo, false);

GrabowskiM · 2021-06-07T13:22:30Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+    });
+
+    doc.addEventListener('scroll', (event) => {
+        const pos = global.scrollY + scrollOffset;


maybe position insead of pos? No need to shorten it

GrabowskiM · 2021-06-07T13:28:34Z

src/bundle/Resources/views/themes/admin/content/edit/base.html.twig

@@ -105,4 +141,4 @@

 {% block stylesheets %}
    {% include '@ezdesign/content/edit/stylesheets.html.twig' %}
-{% endblock %}
+{% endblock %} 


unnecessary change

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

GrabowskiM · 2021-06-09T13:35:07Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+        let endSection = 0;
+        const { anchorTargetSectionId } = event.currentTarget.dataset;
+
+        doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => {


what would you say for sth like

let prevSection = 0; doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => { const { anchorSectionId } = section.dataset; sectionsPosition[anchorSectionId] = prevSection; prevSection = section.offsetHeight; });

as far as I understand you dont need both variables

I am not sure what you are proposing, @GrabowskiM. You only changed startSection name to prevSection and position of its declaration? Maybe I missed something in your code excerpt.

I like change of the variable name, but I wonder, do we even need sectionsPosition, @lucasOsti?
Because it is used only in 1 place AFAIK:
https://github.com/ezsystems/ezplatform-admin-ui/pull/1770/files#diff-2b5ffe8475c06a6575af7905596bab7de43658bb4bd13a9b658da19d8d5de2b1R36
top: sectionsPosition[anchorTargetSectionId],

My proposition in other comment.

GrabowskiM · 2021-06-09T13:39:33Z

src/bundle/Resources/public/js/scripts/admin.context.menu.js

+                    const button = primaryButtons[i];
+                    const isLastButton = i === primaryButtons.length - 1;
+                    const allPreviousButtonVisible = hiddenPrimaryButtons.length === 0;
+                    const isButtonNarrowerThanMoreButton = button.offsetWidth < moreButton.offsetWidth + BUTTON_MARGIN; // ??


src/bundle/Resources/public/js/scripts/admin.content.edit.js

src/bundle/Resources/views/themes/admin/ui/menu/content_edit_menu.html.twig

tischsoic · 2021-06-10T07:58:29Z

src/bundle/Resources/public/js/scripts/admin.content.edit.js

-            const invalidTabLink = doc.querySelector(`a[href="#${invalidTab.id}"]`);
-
-            invalidTabLink.click();
+            invalidButton.click();


Do we click it to basically call navigateTo function from admin.anchor.navigation.js?

tischsoic · 2021-06-10T08:58:07Z

src/bundle/Resources/public/js/scripts/admin.content.edit.js

+                validationResults.filter((result) => !result.isValid).reduce((total, result) => {
+                    total.add(result.validatorName);


For me it is confusing to have reduce, which acts basically as map and at the same time adds items to Set to take only unique results. What do you think about approach, which splits reduce into map and [...new Set()]:

const allValidatorsWithErrors = validationResults.filter((result) => !result.isValid).map((result) => result.validatorName); btn.dataset.validatorsWithErrors = [...new Set(allValidatorsWithErrors)].join();

I have just noticed, that it is old code and you only formatted it. :D
Too much writing to delete it :P

tischsoic · 2021-06-10T09:44:19Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+        let endSection = 0;
+        const { anchorTargetSectionId } = event.currentTarget.dataset;
+
+        doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => {


I am not sure what you are proposing, @GrabowskiM. You only changed startSection name to prevSection and position of its declaration? Maybe I missed something in your code excerpt.

I like change of the variable name, but I wonder, do we even need sectionsPosition, @lucasOsti?
Because it is used only in 1 place AFAIK:
https://github.com/ezsystems/ezplatform-admin-ui/pull/1770/files#diff-2b5ffe8475c06a6575af7905596bab7de43658bb4bd13a9b658da19d8d5de2b1R36
top: sectionsPosition[anchorTargetSectionId],

My proposition in other comment.

tischsoic · 2021-06-10T09:44:24Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+    const navigateTo = (event) => {
+        let startSection = 0;
+        const { anchorTargetSectionId } = event.currentTarget.dataset;
+
+        doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => {
+            const { anchorSectionId } = section.dataset;
+
+            sectionsPosition[anchorSectionId] = startSection;
+            startSection = section.offsetHeight;
+        });
+
+        if (isVerticalScrollVisible()) {
+            formContainerNode.scrollTo({
+                top: sectionsPosition[anchorTargetSectionId],
+                behavior: 'smooth',


What do you think? Am I missing something? @lucasOsti

Suggested change

const navigateTo = (event) => {

let startSection = 0;

const { anchorTargetSectionId } = event.currentTarget.dataset;

doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => {

const { anchorSectionId } = section.dataset;

sectionsPosition[anchorSectionId] = startSection;

startSection = section.offsetHeight;

});

if (isVerticalScrollVisible()) {

formContainerNode.scrollTo({

top: sectionsPosition[anchorTargetSectionId],

behavior: 'smooth',

const navigateTo = (event) => {

const { anchorTargetSectionId } = event.currentTarget.dataset;

const anchorTargetSection = doc.querySelector(`.ibexa-anchor-navigation-sections__section[data-anchor-section-id=${anchorTargetSectionId}]`);

const anchorTargetSectionPreviousSibling = anchorTargetSection.previousSibling;

const scrollTop = anchorTargetSectionPreviousSibling?.offsetHeight ?? 0;

if (isVerticalScrollVisible()) {

formContainerNode.scrollTo({

top: scrollTop,

behavior: 'smooth',

tischsoic · 2021-06-10T10:15:04Z

src/bundle/Resources/public/js/scripts/admin.anchor.navigation.js

+            doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => {
+                const { anchorSectionId } = section.dataset;
+                const start = section.offsetTop;
+                const end = section.offsetHeight + section.offsetTop;
+
+                if (position > start && position < end) {
+                    showSection(anchorSectionId);
+                }
+            });


From my perspective, you basically try to find one and only one section and then call showSection on it, so in my opinion it would be more expressive to do it this way:

Suggested change

doc.querySelectorAll('.ibexa-anchor-navigation-sections__section').forEach((section) => {

const { anchorSectionId } = section.dataset;

const start = section.offsetTop;

const end = section.offsetHeight + section.offsetTop;

if (position > start && position < end) {

showSection(anchorSectionId);

}

});

const allSections = doc.querySelectorAll('.ibexa-anchor-navigation-sections__section');

const activeSection = allSelections.find((section) => {

const { anchorSectionId } = section.dataset;

const start = section.offsetTop;

const end = section.offsetHeight + section.offsetTop;

return position > start && position < end;

});

const activeSectionId = activeSection.dataset.anchorSectionId

showSection(activeSectionId);

dew326 · 2021-07-06T09:12:18Z

src/bundle/Resources/public/scss/_side-menu.scss

+        z-index: 1000;
+    }
+
+    .ez-icon {


Suggested change

.ez-icon {

.ibexa-icon {

?

dew326 · 2021-07-06T09:12:38Z

src/bundle/Resources/public/scss/_side-menu.scss

+            border-bottom-right-radius: $ibexa-border-radius;
+        }
+
+        .ez-icon {


Suggested change

.ez-icon {

.ibexa-icon {

dew326 · 2021-07-06T09:12:49Z

src/bundle/Resources/public/scss/_side-menu.scss

+        }
+    }
+
+    .btn-dark {


probably should not be styled by this class

dew326 · 2021-07-06T09:13:40Z

src/bundle/Resources/views/themes/admin/ui/menu/context_menu.html.twig

+{% endblock %}
+
+{% block label %}
+	<span class="ez-btn__label">{{ parent() }}</span>


Suggested change

<span class="ez-btn__label">{{ parent() }}</span>

<span class="ibexa-btn__label">{{ parent() }}</span>

?

tischsoic · 2021-07-06T11:06:20Z

src/bundle/Resources/public/scss/_extra-actions.scss

@@ -177,3 +177,12 @@
        }
    }
 }
+
+// TO DO: remove when redesigning !!


Should it stay? If so:

Suggested change

// TO DO: remove when redesigning !!

// TODO: remove when redesigning !!

Should it stay? If so:

yes, because after whole redesign we have to change it

tischsoic · 2021-07-06T11:09:51Z

src/bundle/Resources/public/js/scripts/admin.context.menu.js

+            itemHiddenClass: 'ibexa-context-menu__item--hidden',
+            container: adapatItemsContainer,
+            getActiveItem: () => {
+                return adapatItemsContainer.querySelectorAll('.ibexa-context-menu__item')[0];


Maybe 🤔

Suggested change

return adapatItemsContainer.querySelectorAll('.ibexa-context-menu__item')[0];

return adapatItemsContainer.querySelector('.ibexa-context-menu__item');

tischsoic · 2021-07-06T11:10:55Z

src/bundle/Resources/public/js/scripts/admin.context.menu.js

+        const popupMenuElement = adapatItemsContainer.querySelector('.ibexa-popup-menu');
+        const showPopupButton = adapatItemsContainer.querySelector('.ibexa-btn--more');
+        const adaptiveItems = new eZ.core.AdaptiveItems({
+            items: [...adapatItemsContainer.querySelectorAll('.ibexa-context-menu__item:not(.ibexa-context-menu__item--more)')],


You can add a special class to the items and AdaptiveItems will find items on its own.

tischsoic · 2021-07-06T11:13:10Z

src/bundle/Resources/public/js/scripts/admin.context.menu.js

+            position: () => {
+                popupMenuElement.style.right = 0;
+            },


And proper styles in CSS. Would it work?

Suggested change

position: () => {

popupMenuElement.style.right = 0;

},

position: () => {},

tischsoic · 2021-07-06T11:14:52Z

src/bundle/Resources/public/scss/_anchor-navigation.scss

+        border: calculateRem(1px) solid transparent;
+        border-radius: $ibexa-border-radius;
+
+        &:before {


CSS3 syntax - here and in other places

Suggested change

&:before {

&::before {

ref. https://developer.mozilla.org/en-US/docs/Web/CSS/::before#syntax

tischsoic · 2021-07-06T11:29:01Z

src/bundle/Resources/views/themes/admin/ui/component/popup_menu/popup_menu.html.twig

+                'label': label,
+                'class': item.class|default('')
+            }) }}
+		{% endfor %}


Bad indentation?

tischsoic · 2021-07-06T11:30:32Z

src/bundle/Resources/views/themes/admin/ui/component/popup_menu/popup_menu_item.html.twig

@@ -1,3 +1,3 @@
 <li class="ibexa-popup-menu__item {{ class|default('') }}">
-    {{ label }}
+	{{ label }}


Tab instead of 4 spaces? What about other places?

tischsoic · 2021-07-06T11:32:18Z

src/bundle/Resources/views/themes/admin/ui/menu/context_menu.html.twig

+{% extends '@KnpMenu/menu.html.twig' %}
+
+{% block root %}
+	<ul class="ibexa-context-menu">


Tabs should be changed to spaces?

tischsoic · 2021-07-06T11:33:41Z

src/lib/Behat/PageElement/ContentUpdateForm.php

@@ -25,9 +25,9 @@ public function __construct(BrowserContext $context)
        parent::__construct($context);
        $this->fields = [
            'formElement' => '[name=ezplatform_content_forms_content_edit],[name=ezplatform_content_forms_user_create],[name=ezplatform_content_forms_user_update]',
-            'closeButton' => '.ez-content-edit-container__close',
+            'closeButton' => '.ez-content-edit-container__close, .ibexa-content-edit-sidebar__header .ibexa-content-edit-sidebar__back',  // TO DO: Set one selector after redesign


TODO? Here and in other places.

TODO? Here and in other places.

yes, because after whole redesign we have to change it

tischsoic · 2021-07-06T12:17:11Z

src/bundle/Resources/views/themes/admin/ui/menu/context_menu.html.twig

+{% block root %}
+	<ul class="ibexa-context-menu">
+		{% for item in item.children %}
+			{ block('item') }}


Missing {

sonarcloud · 2021-07-12T09:51:40Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
1.9% Duplication

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch from 723d539 to 2db6c51 Compare June 1, 2021 14:05

lucasOsti changed the title ~~Ibx 397 implement the new design for content item editing screen~~ IBX-397: Implement the new design for content item editing screen Jun 2, 2021

lucasOsti requested review from GrabowskiM, dew326 and tischsoic June 4, 2021 13:13

lucasOsti added the Ready for review label Jun 4, 2021

dew326 requested changes Jun 7, 2021

View reviewed changes

GrabowskiM requested changes Jun 7, 2021

View reviewed changes

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch 2 times, most recently from f25465f to 03b091e Compare June 9, 2021 12:07

lucasOsti requested review from dew326 and GrabowskiM June 9, 2021 12:09

GrabowskiM requested changes Jun 9, 2021

View reviewed changes

lucasOsti requested a review from GrabowskiM June 9, 2021 21:19

dew326 requested changes Jun 10, 2021

View reviewed changes

lucasOsti requested a review from dew326 June 10, 2021 06:32

tischsoic reviewed Jun 10, 2021

View reviewed changes

lucasOsti requested a review from tischsoic June 10, 2021 10:39

GrabowskiM approved these changes Jun 11, 2021

View reviewed changes

lucasOsti requested a review from GrabowskiM June 11, 2021 10:36

GrabowskiM approved these changes Jun 11, 2021

View reviewed changes

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch 6 times, most recently from 44efa2a to 0be377a Compare June 30, 2021 10:30

lucasOsti removed the Ready for review label Jun 30, 2021

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch 3 times, most recently from 363bbfd to e8f125a Compare July 1, 2021 11:12

lucasOsti requested a review from GrabowskiM July 1, 2021 11:55

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch 5 times, most recently from 9a3bec0 to 4bd77d4 Compare July 4, 2021 13:28

dew326 requested changes Jul 6, 2021

View reviewed changes

GrabowskiM approved these changes Jul 6, 2021

View reviewed changes

tischsoic requested changes Jul 6, 2021

View reviewed changes

tischsoic reviewed Jul 6, 2021

View reviewed changes

lucasOsti requested review from tischsoic and dew326 July 6, 2021 12:34

lucasOsti added 2 commits July 8, 2021 09:12

Content create redesign draft

0a87f5d

After rebase

2670c61

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch from 026a744 to 2670c61 Compare July 8, 2021 07:13

Temporary fix for behat tests releted with z-index of ez-context-menu

79a1879

tischsoic approved these changes Jul 8, 2021

View reviewed changes

lucasOsti added 2 commits July 8, 2021 09:48

Whitespace fixes

75b85f7

Behat

cce7a08

dew326 approved these changes Jul 8, 2021

View reviewed changes

Behat

889e745

lucasOsti force-pushed the IBX-397-Implement-the-new-design-for-Content-item-editing-screen branch from 0b34056 to 889e745 Compare July 9, 2021 13:25

lucasOsti added 2 commits July 9, 2021 15:28

Behat

321fa1d

Behat

b5bac4a

dew326 merged commit 77e126d into ezsystems:master Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-397: Implement the new design for content item editing screen #1770

IBX-397: Implement the new design for content item editing screen #1770

lucasOsti commented Jun 1, 2021 •

edited

GrabowskiM Jun 7, 2021

GrabowskiM Jun 7, 2021

GrabowskiM Jun 7, 2021

GrabowskiM Jun 7, 2021

GrabowskiM Jun 9, 2021

tischsoic Jun 10, 2021

GrabowskiM Jun 9, 2021

tischsoic Jun 10, 2021

tischsoic Jun 10, 2021

tischsoic Jun 10, 2021

tischsoic Jun 10, 2021

tischsoic Jun 10, 2021

dew326 Jul 6, 2021

dew326 Jul 6, 2021

dew326 Jul 6, 2021

dew326 Jul 6, 2021

tischsoic Jul 6, 2021

lucasOsti Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

tischsoic Jul 6, 2021

lucasOsti Jul 6, 2021

tischsoic Jul 6, 2021

sonarcloud bot commented Jul 12, 2021

	button.addEventListener('click', navigateTo);
	button.addEventListener('click', navigateTo, false);

		validationResults.filter((result) => !result.isValid).reduce((total, result) => {
		total.add(result.validatorName);

	<span class="ez-btn__label">{{ parent() }}</span>
	<span class="ibexa-btn__label">{{ parent() }}</span>

	// TO DO: remove when redesigning !!
	// TODO: remove when redesigning !!

	return adapatItemsContainer.querySelectorAll('.ibexa-context-menu__item')[0];
	return adapatItemsContainer.querySelector('.ibexa-context-menu__item');

IBX-397: Implement the new design for content item editing screen #1770

IBX-397: Implement the new design for content item editing screen #1770

Conversation

lucasOsti commented Jun 1, 2021 • edited

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 12, 2021

lucasOsti commented Jun 1, 2021 •

edited