IBX-617: As an Editor, I want to see the redesigned toggle button #1798

GrabowskiM · 2021-07-09T11:44:42Z

JIRa ref: https://issues.ibexa.co/browse/IBX-617

https://github.com/ezsystems/ezcommerce-shop/pull/326
#1798
https://github.com/ezsystems/ezplatform-form-builder/pull/287
https://github.com/ezsystems/ezplatform-page-builder/pull/783
https://github.com/ezsystems/ezplatform-site-factory/pull/128

dew326 · 2021-07-12T10:00:43Z

src/bundle/Resources/public/js/scripts/fieldType/ezboolean.js

-            const methodName = event.target.checked ? 'add' : 'remove';
-
-            event.target.closest('.ez-data-source__label').classList[methodName]('is-checked');
+            // const methodName = event.target.checked ? 'add' : 'remove';


commented code

dew326 · 2021-07-12T10:01:07Z

src/bundle/Resources/public/js/scripts/fieldType/ezuser.js

-            const label = event.currentTarget.closest(SELECTOR_LABEL);
-
-            label.classList[methodName]('is-checked');
+            // const methodName = event.currentTarget.checked ? 'add' : 'remove';


dew326 · 2021-07-12T10:03:28Z

src/bundle/Resources/public/js/scripts/toggle.button.state.toggle.js

+        event.currentTarget.closest('.ibexa-toggle').classList.remove('ibexa-toggle--is-focused');
+    };
+
+    toggleFields.forEach((toggleField) => toggleField.addEventListener('click', toggleState));


Suggested change

toggleFields.forEach((toggleField) => toggleField.addEventListener('click', toggleState));

toggleFields.forEach((toggleField) => toggleField.addEventListener('click', toggleState, false));

dew326 · 2021-07-12T10:03:48Z

src/bundle/Resources/public/scss/_inputs.scss

+
+    &__input {
+        opacity: 0;
+        height: 1px;


calculateRem

dew326 · 2021-07-12T10:06:22Z

src/bundle/Resources/views/themes/admin/ui/form_fields/toggle_widget.html.twig

+    {% set inputs %}
+    {% endset %}
+
+


Suggested change

dew326 · 2021-07-12T10:06:32Z

src/bundle/Resources/views/themes/admin/ui/form_fields/toggle_widget.html.twig

+            </span>
+        {% endif %}
+    </div>
+{%- endblock -%}


new line at the end

lucasOsti · 2021-07-12T13:00:14Z

src/bundle/Resources/public/scss/_inputs.scss

+        position: absolute;
+        top: calculateRem(3px);
+        left: 0;
+        transition: all 0.3s $ibexa-admin-transition;


use $ibexa-admin-transition-duration instead 0.3

tischsoic · 2021-07-12T12:55:20Z

src/bundle/Resources/public/js/scripts/toggle.button.state.toggle.js

+        event.currentTarget.classList.toggle('ibexa-toggle--is-checked');
+
+        const isChecked = event.currentTarget.classList.contains('ibexa-toggle--is-checked');


I'm not sure if I like this change 🤔 but it can be done this way:

Suggested change

event.currentTarget.classList.toggle('ibexa-toggle--is-checked');

const isChecked = event.currentTarget.classList.contains('ibexa-toggle--is-checked');

const isChecked = event.currentTarget.classList.toggle('ibexa-toggle--is-checked');

ref. https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle#return_value

tischsoic · 2021-07-12T12:57:19Z

src/bundle/Resources/public/js/scripts/toggle.button.state.toggle.js

+        if (event.currentTarget.classList.contains('ibexa-toggle--radio')) {
+            event.currentTarget.querySelector(`.form-check input[value="${valueToSet}"]`).checked = true;
+        } else {
+            event.currentTarget.querySelector('.ibexa-toggle__input').checked = isChecked;


We have a lot of event.currentTarget in this function. We can extract it to a variable at the beginning:

const toggler = event.currentTarget;

tischsoic · 2021-07-12T13:01:36Z

src/bundle/Resources/public/js/scripts/toggle.button.state.toggle.js

+        const valueToSet = isChecked ? 1 : 0;
+
+        if (event.currentTarget.classList.contains('ibexa-toggle--radio')) {
+            event.currentTarget.querySelector(`.form-check input[value="${valueToSet}"]`).checked = true;


As valueToSet is only used in inside first if's execution block.

Suggested change

const valueToSet = isChecked ? 1 : 0;

if (event.currentTarget.classList.contains('ibexa-toggle--radio')) {

event.currentTarget.querySelector(`.form-check input[value="${valueToSet}"]`).checked = true;

if (event.currentTarget.classList.contains('ibexa-toggle--radio')) {

const valueToCheck = isChecked ? 1 : 0;

event.currentTarget.querySelector(`.form-check input[value="${valueToSet}"]`).checked = true;

tischsoic · 2021-07-12T13:05:18Z

src/bundle/Resources/public/js/scripts/toggle.button.state.toggle.js

+            return;
+        }
+
+        event.currentTarget.closest('.ibexa-toggle').classList.add('ibexa-toggle--is-focused');


For me, it looks a little bit complicated. I would split it into two lines.

tischsoic · 2021-07-12T13:07:02Z

src/bundle/Resources/public/js/scripts/toggle.button.state.toggle.js

+    const addFocus = (event) => {
+        event.preventDefault();
+
+        if (event.currentTarget.classList.contains('ibexa-toggle--is-disabled')) {


I think its good to assign conditions to some variables to give them meaning. You can read code faster. :)

Suggested change

if (event.currentTarget.classList.contains('ibexa-toggle--is-disabled')) {

const isTogglerDisabled = event.currentTarget.classList.contains('ibexa-toggle--is-disabled');

if (isTogglerDisabled) {

src/bundle/Resources/views/themes/admin/ui/form_fields/toggle_widget.html.twig

src/bundle/Resources/public/js/scripts/fieldType/ezboolean.js

src/bundle/Resources/public/js/scripts/fieldType/ezuser.js

sonarcloud · 2021-07-20T13:19:04Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

GrabowskiM requested review from lucasOsti, tischsoic and dew326 July 9, 2021 11:45

dew326 requested changes Jul 12, 2021

View reviewed changes

GrabowskiM force-pushed the IBX-617-as-an-editor-i-want-to-see-the-redesigned-toggle-button branch from 61210d3 to d97052a Compare July 12, 2021 10:39

GrabowskiM requested a review from dew326 July 12, 2021 12:48

lucasOsti requested changes Jul 12, 2021

View reviewed changes

tischsoic requested changes Jul 12, 2021

View reviewed changes

GrabowskiM force-pushed the IBX-617-as-an-editor-i-want-to-see-the-redesigned-toggle-button branch from d66f52f to ce62098 Compare July 14, 2021 10:43

GrabowskiM requested review from tischsoic and lucasOsti July 14, 2021 10:43

GrabowskiM force-pushed the IBX-617-as-an-editor-i-want-to-see-the-redesigned-toggle-button branch from ce62098 to 947bbb9 Compare July 15, 2021 07:38

lucasOsti approved these changes Jul 15, 2021

View reviewed changes

GrabowskiM force-pushed the IBX-617-as-an-editor-i-want-to-see-the-redesigned-toggle-button branch from 947bbb9 to 5c2bd8f Compare July 15, 2021 08:54

dew326 requested changes Jul 15, 2021

View reviewed changes

src/bundle/Resources/public/js/scripts/fieldType/ezboolean.js Show resolved Hide resolved

src/bundle/Resources/public/js/scripts/fieldType/ezuser.js Show resolved Hide resolved

GrabowskiM requested a review from dew326 July 15, 2021 10:36

dew326 approved these changes Jul 15, 2021

View reviewed changes

Michał Grabowski and others added 6 commits July 20, 2021 15:16

IBX-617: As an Editor, I want to see the redesigned toggle button

2597b4f

IBX-617: After CR

16088af

[Behat] Adapted User creation test

71425e5

IBX-617: After CR

acfde14

IBX-617: Removed updateState from eventMap

1f2b2b4

[Behat] Fixed tests

15d9afd

GrabowskiM force-pushed the IBX-617-as-an-editor-i-want-to-see-the-redesigned-toggle-button branch from 5c04e52 to 15d9afd Compare July 20, 2021 13:16

dew326 merged commit c579183 into master Jul 21, 2021

dew326 deleted the IBX-617-as-an-editor-i-want-to-see-the-redesigned-toggle-button branch July 21, 2021 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-617: As an Editor, I want to see the redesigned toggle button #1798

IBX-617: As an Editor, I want to see the redesigned toggle button #1798

GrabowskiM commented Jul 9, 2021 •

edited

dew326 Jul 12, 2021

dew326 Jul 12, 2021

dew326 Jul 12, 2021

dew326 Jul 12, 2021

dew326 Jul 12, 2021

dew326 Jul 12, 2021

lucasOsti Jul 12, 2021

tischsoic Jul 12, 2021

tischsoic Jul 12, 2021

tischsoic Jul 12, 2021

tischsoic Jul 12, 2021

tischsoic Jul 12, 2021

sonarcloud bot commented Jul 20, 2021

	toggleFields.forEach((toggleField) => toggleField.addEventListener('click', toggleState));
	toggleFields.forEach((toggleField) => toggleField.addEventListener('click', toggleState, false));

		event.currentTarget.classList.toggle('ibexa-toggle--is-checked');

		const isChecked = event.currentTarget.classList.contains('ibexa-toggle--is-checked');

-        if (event.currentTarget.classList.contains('ibexa-toggle--is-disabled')) {
+        const isTogglerDisabled = event.currentTarget.classList.contains('ibexa-toggle--is-disabled');
+        if (isTogglerDisabled) {

IBX-617: As an Editor, I want to see the redesigned toggle button #1798

IBX-617: As an Editor, I want to see the redesigned toggle button #1798

Conversation

GrabowskiM commented Jul 9, 2021 • edited

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 20, 2021

GrabowskiM commented Jul 9, 2021 •

edited