From 9a207c8e50d44e7246c4636f4eba7125c1e9acb5 Mon Sep 17 00:00:00 2001 From: "Robin Lejeune (role)" Date: Wed, 31 Jan 2024 12:54:58 +0100 Subject: [PATCH] [FIX] website, web_editor: fix mobile orders > 12 Commit [1] introduced mobile orders for columns in flex containers snippets. This was later amended with commit [2] to use Bootstrap's `order-X` classes. Finally, to be complete, commit [3] also added some manipulations around mobile orders. Those classes are limited to 12 possible orders, which means the feature stops working for any column over that threshold: a column with `order-13` will appear as if it didn't have any order. In the end, it has been decided that the trade-off of being capped at 12 mobile orders (and so 12 columns) and the behavior it causes isn't worth using the classes: we will use inline style instead. [1]: https://github.com/odoo/odoo/commit/710d000f1872fd99b41d52ec3d6923756bba7cba [2]: https://github.com/odoo/odoo/commit/143bdfa13d331b93b88e207f181840d91796cdce [3]: https://github.com/odoo/odoo/commit/7b27385dba36c2e741d96e76ad5d847d09f2b084 task-3666688 closes odoo/odoo#152024 Signed-off-by: Benoit Socias (bso) --- .../src/js/common/column_layout_mixin.js | 38 +++++++---- .../static/src/js/editor/snippets.editor.js | 4 +- .../static/src/js/editor/snippets.options.js | 64 +++++++++++-------- .../tours/website_update_column_count.js | 14 ++-- 4 files changed, 69 insertions(+), 51 deletions(-) diff --git a/addons/web_editor/static/src/js/common/column_layout_mixin.js b/addons/web_editor/static/src/js/common/column_layout_mixin.js index 0c0c1512a86..a32040576cb 100644 --- a/addons/web_editor/static/src/js/common/column_layout_mixin.js +++ b/addons/web_editor/static/src/js/common/column_layout_mixin.js @@ -27,17 +27,19 @@ export const ColumnLayoutMixin = { return Math.floor((12 - offsetSize) / colSize); }, /** - * Retrieves the mobile order class as a match array if there is one. + * TODO: remove in master + * Retrieves the mobile order as a match array if there is one. * * @private * @param {HTMLElement} el * @returns {Array|null} class match ["order-XX", "XX"] */ _getItemMobileOrder(el) { - return el.className.match(/\border-([0-9]+)\b/); + const order = el.style.order; + return order ? [`order-${order}`, order.toString()] : null; }, /** - * Gets the first item, whether it has a mobile order class or not. + * Gets the first item, whether it has a mobile order or not. * * @private * @param {HTMLCollection} columnEls - elements in the .row container @@ -45,19 +47,30 @@ export const ColumnLayoutMixin = { * @returns {HTMLElement} first HTMLElement in order */ _getFirstItem(columnEls, isMobile) { - return isMobile && [...columnEls].find(el => el.classList.contains("order-0")) - || columnEls[0]; + return isMobile && [...columnEls].find(el => el.style.order === "0") || columnEls[0]; }, /** - * Adds the classes for mobile order. + * Adds mobile order and the reset class for large screens. * * @private * @param {HTMLCollection} columnEls - elements in the .row container */ _addMobileOrders(columnEls) { for (let i = 0; i < columnEls.length; i++) { - const mobileOrderClass = `order-${i}`; - columnEls[i].classList.add(mobileOrderClass, "order-lg-0"); + columnEls[i].style.order = i; + columnEls[i].classList.add("order-lg-0"); + } + }, + /** + * Removes mobile orders and the reset class for large screens. + * + * @private + * @param {HTMLCollection} columnEls - elements in the .row container + */ + _removeMobileOrders(columnEls) { + for (const el of columnEls) { + el.style.order = ""; + el.classList.remove("order-lg-0"); } }, /** @@ -104,12 +117,9 @@ export const ColumnLayoutMixin = { */ _fillRemovedItemGap(parentEl, itemOrder) { [...parentEl.children].forEach(el => { - const elMobileOrder = this._getItemMobileOrder(el); - if (elMobileOrder) { - const elOrder = parseInt(elMobileOrder[1]); - if (elOrder > itemOrder) { - el.classList.replace(`order-${elOrder}`, `order-${elOrder - 1}`); - } + const elOrder = parseInt(el.style.order); + if (elOrder > itemOrder) { + el.style.order = elOrder - 1; } }); }, diff --git a/addons/web_editor/static/src/js/editor/snippets.editor.js b/addons/web_editor/static/src/js/editor/snippets.editor.js index 2d60f2bbf7e..e5eb2b35ca9 100644 --- a/addons/web_editor/static/src/js/editor/snippets.editor.js +++ b/addons/web_editor/static/src/js/editor/snippets.editor.js @@ -1095,10 +1095,10 @@ var SnippetEditor = Widget.extend({ } // If the target has a mobile order class, store its parent and order. - const targetMobileOrder = ColumnLayoutMixin._getItemMobileOrder(this.$target[0]) + const targetMobileOrder = this.$target[0].style.order; if (targetMobileOrder) { this.dragState.startingParent = this.$target[0].parentNode; - this.dragState.mobileOrder = parseInt(targetMobileOrder[1]); + this.dragState.mobileOrder = parseInt(targetMobileOrder); } const toInsertInline = window.getComputedStyle(this.$target[0]).display.includes('inline'); diff --git a/addons/web_editor/static/src/js/editor/snippets.options.js b/addons/web_editor/static/src/js/editor/snippets.options.js index 2c03c4967a3..2208c30db70 100644 --- a/addons/web_editor/static/src/js/editor/snippets.options.js +++ b/addons/web_editor/static/src/js/editor/snippets.options.js @@ -5671,17 +5671,30 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { $overlayArea.prepend($buttons[0]); // Needed for compatibility (with already dropped snippets). - // If the target is a column, check if all the columns are either mobile - // ordered or not. If they are not consistent, then we remove the mobile - // order classes from all of them, to avoid issues. const parentEl = this.$target[0].parentElement; if (parentEl.classList.contains("row")) { const columnEls = [...parentEl.children]; - const orderedColumnEls = columnEls.filter(el => this._getItemMobileOrder(el)); + let orderedColumnEls = columnEls.filter(el => el.style.order); + + // TODO: remove in master once handled by a migration script. + // If there is no inline order, make sure that any existing order + // class is replaced with inline CSS. + if (!orderedColumnEls.length) { + for (const el of columnEls) { + const orderClass = el.className.match(/(^|\s+)(?order-(?[0-9]+))(?!\S)/); + if (orderClass) { + el.classList.remove(orderClass.groups.cls); + el.style.order = orderClass.groups.ord; + orderedColumnEls.push(el); + } + } + } + + // If the target is a column, check if all the columns are either + // mobile ordered or not. If they are not consistent, then we remove + // the mobile order classes from all of them, to avoid issues. if (orderedColumnEls.length && orderedColumnEls.length !== columnEls.length) { - orderedColumnEls.forEach(el => { - el.className = el.className.replace(/\border(-lg)?-[0-9]+\b/g, ""); - }); + this._removeMobileOrders(orderedColumnEls); } } @@ -5692,20 +5705,19 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { */ onClone(options) { this._super.apply(this, arguments); - const mobileOrder = this._getItemMobileOrder(this.$target[0]); + const mobileOrder = this.$target[0].style.order; // If the order has been adapted on mobile, it must be different // for each clone. if (options.isCurrent && mobileOrder) { const siblingEls = this.$target[0].parentElement.children; - const cloneEls = [...siblingEls].filter(el => el.classList.contains(mobileOrder[0])); + const cloneEls = [...siblingEls].filter(el => el.style.order === mobileOrder); // For cases in which multiple clones are made at the same time, we // change the order for all clones at once. (e.g.: it happens when // increasing the columns count.) This makes sure the clones get a // mobile order in line with their DOM order. cloneEls.forEach((el, i) => { if (i > 0) { - const newMobileOrder = siblingEls.length - cloneEls.length + i; - el.classList.replace(mobileOrder[0], `order-${newMobileOrder}`); + el.style.order = siblingEls.length - cloneEls.length + i; } }); } @@ -5716,20 +5728,18 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { onMove() { this._super.apply(this, arguments); // Remove all the mobile order classes after a drag and drop. - [...this.$target[0].parentElement.children].forEach(el => { - el.className = el.className.replace(/\border(-lg)?-[0-9]+\b/g, ""); - }); + this._removeMobileOrders(this.$target[0].parentElement.children); }, /** * @override */ onRemove() { this._super.apply(this, arguments); - const targetMobileOrder = this._getItemMobileOrder(this.$target[0]); + const targetMobileOrder = this.$target[0].style.order; // If the order has been adapted on mobile, the gap created by the // removed snippet must be filled in. if (targetMobileOrder) { - const targetOrder = parseInt(targetMobileOrder[1]); + const targetOrder = parseInt(targetMobileOrder); this._fillRemovedItemGap(this.$target[0].parentElement, targetOrder); } }, @@ -5752,7 +5762,7 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { let siblingEls, mobileOrder; if (moveLeftOrRight) { siblingEls = this.$target[0].parentElement.children; - mobileOrder = !!this._getItemMobileOrder(this.$target[0]); + mobileOrder = !!this.$target[0].style.order; } if (moveLeftOrRight && isMobile && !isNavItem) { if (!mobileOrder) { @@ -5787,9 +5797,7 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { } } if (mobileOrder) { - for (const el of siblingEls) { - el.className = el.className.replace(/\border(-lg)?-[0-9]+\b/g, ""); - } + this._removeMobileOrders(siblingEls); } } if (!this.$target.is(this.data.noScroll) @@ -5839,15 +5847,15 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { // On mobile, items' reordering is independent from desktop inside // a snippet (left or right), not at a higher level (up or down). if (moveLeftOrRight && isMobileView) { - const targetMobileOrder = this._getItemMobileOrder(this.$target[0]); + const targetMobileOrder = this.$target[0].style.order; if (targetMobileOrder) { const siblingEls = this.$target[0].parentElement.children; const orderModifier = widgetName === "move_left_opt" ? -1 : 1; let delta = 0; while (true) { delta += orderModifier; - const nextOrderClass = `order-${parseInt(targetMobileOrder[1]) + delta}`; - const siblingEl = [...siblingEls].find(el => el.classList.contains(nextOrderClass)); + const nextOrder = parseInt(targetMobileOrder) + delta; + const siblingEl = [...siblingEls].find(el => el.style.order === nextOrder.toString()); if (!siblingEl) { break; } @@ -5876,13 +5884,13 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { * @param {HTMLCollection} siblingEls */ _swapMobileOrders(widgetValue, siblingEls) { - const targetMobileOrder = this._getItemMobileOrder(this.$target[0]); + const targetMobileOrder = this.$target[0].style.order; const orderModifier = widgetValue === "prev" ? -1 : 1; let delta = 0; while (true) { delta += orderModifier; - const newOrderClass = `order-${parseInt(targetMobileOrder[1]) + delta}`; - const comparedEl = [...siblingEls].find(el => el.classList.contains(newOrderClass)); + const newOrder = parseInt(targetMobileOrder) + delta; + const comparedEl = [...siblingEls].find(el => el.style.order === newOrder.toString()); // TODO In master: remove break. if (!comparedEl) { break; @@ -5890,8 +5898,8 @@ registry.SnippetMove = SnippetOptionWidget.extend(ColumnLayoutMixin, { if (window.getComputedStyle(comparedEl).display === "none") { continue; } - this.$target[0].classList.replace(targetMobileOrder[0], newOrderClass); - comparedEl.classList.replace(newOrderClass, targetMobileOrder[0]); + this.$target[0].style.order = newOrder; + comparedEl.style.order = targetMobileOrder; break; } }, diff --git a/addons/website/static/tests/tours/website_update_column_count.js b/addons/website/static/tests/tours/website_update_column_count.js index 947f3203df8..3a23f18d9c1 100644 --- a/addons/website/static/tests/tours/website_update_column_count.js +++ b/addons/website/static/tests/tours/website_update_column_count.js @@ -80,10 +80,10 @@ wTourUtils.clickOnSnippet({ content: "Change the orders of the 2nd and 3rd items", trigger: "iframe .o_overlay_move_options [data-name='move_right_opt']", }, { - content: "Check that the 1st item now has a class .order-0.order-lg-0" + - "and that .order-1.order-lg-0 is set on the 3rd item, and .order-2.order-lg-0 on the 2nd", - trigger: `${columnsSnippetRow}:has(.order-0.order-lg-0:first-child)`, - extra_trigger: `${columnsSnippetRow}:has(.order-2.order-lg-0:nth-child(2) + .order-1.order-lg-0:nth-child(3))`, + content: "Check that the 1st item now has order: 0 and a class .order-lg-0 " + + "and that order: 1, .order-lg-0 is set on the 3rd item, and order: 2, .order-lg-0 on the 2nd", + trigger: `${columnsSnippetRow}:has([style*='order: 0;'].order-lg-0:first-child)`, + extra_trigger: `${columnsSnippetRow}:has([style*='order: 2;'].order-lg-0:nth-child(2) + [style*='order: 1;'].order-lg-0:nth-child(3))`, isCheck: true, }, { content: "Toggle desktop view", @@ -96,7 +96,7 @@ wTourUtils.clickOnSnippet({ trigger: `${columnCountOptSelector} we-button[data-select-count='6']`, }, { content: "Check that each item has a different mobile order from 0 to 5", - trigger: `${columnsSnippetRow}${[0, 1, 2, 3, 4, 5].map(n => `:has(.order-${n}.order-lg-0)`).join("")}`, + trigger: `${columnsSnippetRow}${[0, 1, 2, 3, 4, 5].map(n => `:has([style*='order: ${n};'].order-lg-0)`).join("")}`, isCheck: true, }, { content: "Click on the 6th item", @@ -111,8 +111,8 @@ wTourUtils.clickOnSnippet({ content: "Change the orders of the 5th and 6th items to override the mobile orders", trigger: "iframe .o_overlay_move_options [data-name='move_left_opt']", }, { - content: "Check that there are no .order-X classes anymore", - trigger: `${columnsSnippetRow}:not(:has(.order-0)):not(:has(.order-lg-0))`, + content: "Check that there are no orders anymore", + trigger: `${columnsSnippetRow}:not(:has([style*='order: 0;'])):not(:has(.order-lg-0))`, isCheck: true, }, ]);