From 35253205907a16c391cb8015043e607e5ee6e117 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Thu, 21 Sep 2023 14:22:42 +0200 Subject: [PATCH 1/7] We should use `cable_ready.inner_html` instead of `morph` To be sure that every stimulus controllers attached to the DOM that is replaced is will connected. What I saw is that when using `morph`, the stimulus controller (specially `tooltip_controller`) were not attached to the DOM (and not disconnected as well) that triggered some errors in the console. Adds test case for payment capture thanks to @filipefurtad0 --- app/reflexes/admin/orders_reflex.rb | 9 +++++++-- spec/system/admin/orders_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/app/reflexes/admin/orders_reflex.rb b/app/reflexes/admin/orders_reflex.rb index 765438d7cc..fe59036fb8 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -8,8 +8,13 @@ module Admin payment_capture = OrderCaptureService.new(@order) if payment_capture.call - morph dom_id(@order), render(partial: "spree/admin/orders/table_row", - locals: { order: @order.reload, success: true }) + cable_ready.inner_html( + selector: dom_id(@order), + html: render(partial: "spree/admin/orders/table_row", + locals: { order: @order.reload, success: true }) + ).broadcast + + morph :nothing else flash[:error] = payment_capture.gateway_error || I18n.t(:payment_processing_failed) morph_admin_flashes diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 78ac3ba5b5..342cdda0a0 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -750,6 +750,28 @@ describe ' end end + it "displays Ship and Edit tooltips, after capturing a payment" do + within "tr#order_#{order.id}" do + # checks the order has an uncaptured payment + find(".icon-capture").hover + expect(page).to have_content "Capture" + + # captures the payment + find(".icon-capture").click + expect(page).not_to have_content "Capture" + + # checks shipment state + expect(page).to have_content "READY" + # mouse-hovers and finds Ship tooltip + find(".icon-road").hover + expect(page).to have_content "Ship" + + # mouse-hovers and finds Edit tooltip + find(".icon-edit").hover + expect(page).to have_content "Edit" + end + end + it "displays Edit tooltip" do within "tr#order_#{order.id}" do # checks shipment state From 31c537c5e095cf72d8aaa9cf86b6ad9f6680ee97 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 25 Sep 2023 13:42:35 +0200 Subject: [PATCH 2/7] Revert to using `morph` instead of `cable_ready` The issue is with with the stimilus tooltip controller, it add some element to the DOM which create issue when it's modified by StimulusReflex. See here for a more detailed explanation: https://github.com/stimulusreflex/stimulus_reflex/issues/314#issuecomment-702479357 --- app/reflexes/admin/orders_reflex.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/app/reflexes/admin/orders_reflex.rb b/app/reflexes/admin/orders_reflex.rb index fe59036fb8..765438d7cc 100644 --- a/app/reflexes/admin/orders_reflex.rb +++ b/app/reflexes/admin/orders_reflex.rb @@ -8,13 +8,8 @@ module Admin payment_capture = OrderCaptureService.new(@order) if payment_capture.call - cable_ready.inner_html( - selector: dom_id(@order), - html: render(partial: "spree/admin/orders/table_row", - locals: { order: @order.reload, success: true }) - ).broadcast - - morph :nothing + morph dom_id(@order), render(partial: "spree/admin/orders/table_row", + locals: { order: @order.reload, success: true }) else flash[:error] = payment_capture.gateway_error || I18n.t(:payment_processing_failed) morph_admin_flashes From 4639e53673d74eac9c0b9085c0131b4d1f8b7ebb Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 25 Sep 2023 13:45:44 +0200 Subject: [PATCH 3/7] Remove `insertToolTipMarkup` It's not great to have Stimulus controller rendering markup on `connect` Stimulus is intended to add behavior to existing markup. Plus add some documentation --- .../controllers/tooltip_controller.js | 37 ++++++++----------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/app/webpacker/controllers/tooltip_controller.js b/app/webpacker/controllers/tooltip_controller.js index b75e7eab6c..8ce57242ad 100644 --- a/app/webpacker/controllers/tooltip_controller.js +++ b/app/webpacker/controllers/tooltip_controller.js @@ -1,16 +1,28 @@ import { Controller } from "stimulus"; import { computePosition, offset, arrow } from "@floating-ui/dom"; +// This is meant to be used with the follwing html where element can be a +// "div", "a", "span" or "button", etc... : +// +//
+// +//
+//
+// tooltip_text +//
+//
+//
+//
+// +// You can also use this partial app/views/admin/shared/_tooltip.html.haml + export default class extends Controller { static targets = ["element", "tooltip", "arrow"]; static values = { - tip: String, placement: { type: String, default: "top" }, }; connect() { - if (this.hasTipValue) { this.insertToolTipMarkup() } - this.elementTarget.addEventListener("mouseenter", this.showTooltip); this.elementTarget.addEventListener("mouseleave", this.hideTooltip); } @@ -56,23 +68,4 @@ export default class extends Controller { hideTooltip = () => { this.tooltipTarget.style.display = ""; }; - - insertToolTipMarkup() { - let container = document.createElement("div"); - let tooltip = document.createElement("div"); - let arrow = document.createElement("div"); - let text = document.createTextNode(this.tipValue); - - container.classList.add("tooltip-container"); - tooltip.classList.add("tooltip"); - tooltip.setAttribute("data-tooltip-target", "tooltip"); - arrow.classList.add("arrow"); - arrow.setAttribute("data-tooltip-target", "arrow"); - - container.appendChild(tooltip); - tooltip.appendChild(text); - tooltip.appendChild(arrow); - - this.elementTarget.appendChild(container); - } } From 01a13a814a418267f7f7aaae672c23bcbd77b855 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 25 Sep 2023 14:05:40 +0200 Subject: [PATCH 4/7] Improve tooltip partial --- app/views/admin/shared/_tooltip.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/shared/_tooltip.html.haml b/app/views/admin/shared/_tooltip.html.haml index 0f193e60b9..6e28c08c43 100644 --- a/app/views/admin/shared/_tooltip.html.haml +++ b/app/views/admin/shared/_tooltip.html.haml @@ -1,6 +1,6 @@ %div{"data-controller": "tooltip"} - %a{"data-tooltip-target": "element"} - = t('admin.whats_this') + %a{"data-tooltip-target": "element", href: link, class: link_class} + = link_text .tooltip-container .tooltip{"data-tooltip-target": "tooltip"} = sanitize tooltip_text From 6a1664d2fdf5de86d1309d6a1ca4cbb87fc10642 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 25 Sep 2023 14:08:02 +0200 Subject: [PATCH 5/7] Refactor "What's this" tooltip --- app/views/admin/enterprises/form/_address.html.haml | 2 +- .../admin/enterprises/form/_business_details.html.haml | 2 +- app/views/admin/enterprises/form/_contact.html.haml | 4 ++-- app/views/admin/enterprises/form/_permalink.html.haml | 6 +++--- .../admin/enterprises/form/_primary_details.html.haml | 8 ++++---- app/views/admin/enterprises/form/_users.html.haml | 8 ++++---- app/views/admin/shared/_whats_this_tooltip.html.haml | 1 + 7 files changed, 16 insertions(+), 15 deletions(-) create mode 100644 app/views/admin/shared/_whats_this_tooltip.html.haml diff --git a/app/views/admin/enterprises/form/_address.html.haml b/app/views/admin/enterprises/form/_address.html.haml index 6ce9fd1411..9d76513a3d 100644 --- a/app/views/admin/enterprises/form/_address.html.haml +++ b/app/views/admin/enterprises/form/_address.html.haml @@ -41,7 +41,7 @@ = af.label :latitude, t(:latitude) \/ = af.label :longitude, t(:longitude) - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('latitude_longitude_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('latitude_longitude_tip')} .four.columns = af.text_field :latitude, { placeholder: t(:latitude_placeholder) } .four.columns.omega diff --git a/app/views/admin/enterprises/form/_business_details.html.haml b/app/views/admin/enterprises/form/_business_details.html.haml index d91d19924f..30bef3ba2f 100644 --- a/app/views/admin/enterprises/form/_business_details.html.haml +++ b/app/views/admin/enterprises/form/_business_details.html.haml @@ -80,7 +80,7 @@ %legend= t('.invoice_item_sorting_legend') .three.columns.alpha %label= t('.sort_items_by_supplier?') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.sort_items_by_supplier_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.sort_items_by_supplier_tip')} .three.columns = f.radio_button :preferred_invoice_order_by_supplier, true = f.label :preffered_invoice_order_by_supplier, t('.enabled'), value: :true diff --git a/app/views/admin/enterprises/form/_contact.html.haml b/app/views/admin/enterprises/form/_contact.html.haml index f8c3e75a9c..63080f7992 100644 --- a/app/views/admin/enterprises/form/_contact.html.haml +++ b/app/views/admin/enterprises/form/_contact.html.haml @@ -6,7 +6,7 @@ .row .alpha.three.columns = f.label :email_address, t('.email_address') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.email_address_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.email_address_tip')} .omega.eight.columns = f.text_field :email_address, { placeholder: t('.email_address_placeholder') } @@ -18,7 +18,7 @@ .row .alpha.three.columns = f.label :whatsapp_phone, t('.whatsapp_phone') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.whatsapp_phone_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.whatsapp_phone_tip')} .omega.eight.columns = f.text_field :whatsapp_phone, { placeholder: t('.whatsapp_phone_placeholder') } diff --git a/app/views/admin/enterprises/form/_permalink.html.haml b/app/views/admin/enterprises/form/_permalink.html.haml index 28867e9392..5c2a43a7b0 100644 --- a/app/views/admin/enterprises/form/_permalink.html.haml +++ b/app/views/admin/enterprises/form/_permalink.html.haml @@ -3,7 +3,7 @@ .row .three.columns.alpha = label_tag :permalink, t('.permalink') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.permalink_tip', link: main_app.root_url)} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.permalink_tip', link: main_app.root_url)} .eight.columns = text_field_tag "enterprise[permalink]", @enterprise.permalink, data: { action: "input->permalink#validate", "permalink-target": "permalinkField" } .two.columns.omega @@ -20,13 +20,13 @@ .row .three.columns.alpha %label= t('.link_to_front') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.link_to_front_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.link_to_front_tip')} .eight.columns.omega - front_shop_path = "#{main_app.root_url}#{@enterprise.permalink}/shop" = link_to front_shop_path, front_shop_path , target: "_blank" .row .three.columns.alpha = label_tag :id, t('.ofn_uid') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.ofn_uid_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.ofn_uid_tip')} .six.columns = @enterprise.id diff --git a/app/views/admin/enterprises/form/_primary_details.html.haml b/app/views/admin/enterprises/form/_primary_details.html.haml index e4269c8475..04609e9b58 100644 --- a/app/views/admin/enterprises/form/_primary_details.html.haml +++ b/app/views/admin/enterprises/form/_primary_details.html.haml @@ -8,13 +8,13 @@ .row .three.columns.alpha = f.label :group_ids, t('.groups') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.groups_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.groups_tip')} .eight.columns.omega = f.collection_select :group_ids, @groups, :id, :name, {}, data: { controller: "tom-select", "tom-select-options-value": { plugins: ['remove_button'], maxItems: nil } }, class: "full-width", multiple: true, placeholder: t('.groups_placeholder') .row .three.columns.alpha %label= t('.primary_producer') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.primary_producer_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.primary_producer_tip')} .five.columns.omega = f.check_box :is_primary_producer, data: { action: "change->primary-details#primaryProducerChanged" } = f.label :is_primary_producer, t('.producer') @@ -22,7 +22,7 @@ .row .three.columns.alpha = f.label :sells, t('.sells') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.sells_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.sells_tip')} .two.columns = f.radio_button :sells, "none", 'ng-model' => 'Enterprise.sells', data: {action: "change->primary-details#enterpriseSellsChanged"} = f.label :sells, t('.none'), value: "none" @@ -37,7 +37,7 @@ .row .three.columns.alpha %label= t('.visible_in_search') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.visible_in_search_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.visible_in_search_tip')} .two.columns = f.radio_button :visible, "public", 'ng-model' => 'Enterprise.visible' = f.label :visible, t('.visible'), value: 'public' diff --git a/app/views/admin/enterprises/form/_users.html.haml b/app/views/admin/enterprises/form/_users.html.haml index 2579b35369..8d33a27132 100644 --- a/app/views/admin/enterprises/form/_users.html.haml +++ b/app/views/admin/enterprises/form/_users.html.haml @@ -6,7 +6,7 @@ =f.label :owner_id, t('.owner') - if full_permissions %span.required * - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.owner_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.owner_tip')} .eight.columns.omega - if full_permissions = f.hidden_field :owner_id, class: "select2 fullwidth", 'user-select' => 'Enterprise.owner', 'ng-model' => 'Enterprise.owner' @@ -18,7 +18,7 @@ =f.label :user_ids, t('.notifications') - if full_permissions %span.required * - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.contact_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.contact_tip')} .eight.columns.omega - if full_permissions %select.select2.fullwidth{id: 'receives_notifications_dropdown', name: 'receives_notifications', ng: {model: 'receivesNotifications', init: "receivesNotifications = '#{@enterprise.contact.id}'"}} @@ -32,7 +32,7 @@ =f.label :user_ids, t('.managers') - if full_permissions %span.required * - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.managers_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.managers_tip')} .eight.columns.omega - if full_permissions %table.managers @@ -63,7 +63,7 @@ .three.columns.alpha %label = t('.invite_manager') - = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.invite_manager_tip')} + = render partial: 'admin/shared/whats_this_tooltip', locals: {tooltip_text: t('.invite_manager_tip')} .eight.columns.omega .row %a.button{ "data-controller": "help-modal-link", "data-action": "click->help-modal-link#open", "data-help-modal-link-target-value": "invite-manager-modal" } diff --git a/app/views/admin/shared/_whats_this_tooltip.html.haml b/app/views/admin/shared/_whats_this_tooltip.html.haml new file mode 100644 index 0000000000..d63fa41768 --- /dev/null +++ b/app/views/admin/shared/_whats_this_tooltip.html.haml @@ -0,0 +1 @@ += render partial: 'admin/shared/tooltip', locals: {link_class: "" ,link: nil, link_text: t('admin.whats_this'), tooltip_text: tooltip_text} From 7ce3fea7d5dd4401b7d928902a753ba30c377d82 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 25 Sep 2023 15:09:53 +0200 Subject: [PATCH 6/7] Refactor tooltip on the backoffice orders page --- app/views/admin/shared/_tooltip_button.html.haml | 7 +++++++ app/views/spree/admin/orders/_table_row.html.haml | 15 +++++++-------- spec/system/admin/orders_spec.rb | 10 ++++++++-- 3 files changed, 22 insertions(+), 10 deletions(-) create mode 100644 app/views/admin/shared/_tooltip_button.html.haml diff --git a/app/views/admin/shared/_tooltip_button.html.haml b/app/views/admin/shared/_tooltip_button.html.haml new file mode 100644 index 0000000000..06b37688cd --- /dev/null +++ b/app/views/admin/shared/_tooltip_button.html.haml @@ -0,0 +1,7 @@ +%div{ "data-controller": "tooltip" } + %button{class: button_class, "data-reflex": button_reflex, "data-id": reflex_data_id, "data-tooltip-target": "element" } + .tooltip-container + .tooltip{"data-tooltip-target": "tooltip"} + = sanitize tooltip_text + .arrow{"data-tooltip-target": "arrow"} + diff --git a/app/views/spree/admin/orders/_table_row.html.haml b/app/views/spree/admin/orders/_table_row.html.haml index aa72f0823a..9c0ffb5e17 100644 --- a/app/views/spree/admin/orders/_table_row.html.haml +++ b/app/views/spree/admin/orders/_table_row.html.haml @@ -14,6 +14,10 @@ %div{ "data-controller": "tooltip", "data-tooltip-tip-value": order.special_instructions.to_s } %span.icon-warning-sign{ "data-tooltip-target": "element" } = t('spree.admin.orders.index.note') + .tooltip-container + .tooltip{"data-tooltip-target": "tooltip"} + = sanitize order.special_instructions.to_s + .arrow{"data-tooltip-target": "arrow"} %td.align-left %span.state{ class: order.state.to_s } = t('js.admin.orders.order_state.' + order.state.to_s) @@ -41,13 +45,8 @@ %div.row-loading-icons - if local_assigns[:success] %i.success.icon-ok-sign{"data-controller": "ephemeral"} - %a.icon_link.with-tip.icon-edit.no-text{href: edit_admin_order_path(order), "data-controller": "tooltip", "data-tooltip-tip-value": t('spree.admin.orders.index.edit'), "data-tooltip-target": "element"} + = render partial: 'admin/shared/tooltip', locals: {link_class: "icon_link with-tip icon-edit no-text" ,link: edit_admin_order_path(order), link_text: "", tooltip_text: t('spree.admin.orders.index.edit')} - if order.ready_to_ship? - %div{ "data-controller": "tooltip", "data-tooltip-tip-value": t('spree.admin.orders.index.ship') } - %button.icon-road.icon_link.with-tip.no-text{"data-reflex": "click->Admin::OrdersReflex#ship", "data-id": order.id.to_s, - "data-tooltip-target": "element" } - + = render partial: 'admin/shared/tooltip_button', locals: {button_class: "icon-road icon_link with-tip no-text", button_reflex: "click->Admin::OrdersReflex#ship", reflex_data_id: order.id.to_s, tooltip_text: t('spree.admin.orders.index.ship')} - if order.payment_required? && order.pending_payments.reject(&:requires_authorization?).any? - %div{ "data-controller": "tooltip", "data-tooltip-tip-value": t('spree.admin.orders.index.capture') } - %button.icon-capture.icon_link.no-text{"data-reflex": "click->Admin::OrdersReflex#capture", "data-id": order.id.to_s, - "data-tooltip-target": "element" } + = render partial: 'admin/shared/tooltip_button', locals: {button_class: "icon-capture icon_link no-text", button_reflex: "click->Admin::OrdersReflex#capture", reflex_data_id: order.id.to_s, tooltip_text: t('spree.admin.orders.index.capture')} diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 342cdda0a0..8c8b388d63 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -443,8 +443,9 @@ describe ' it "displays a note with order instructions" do within "tr#order_#{order3.id}" do - expect(page).to have_content I18n.t('spree.admin.orders.index.note') - expect(page).to have_css "[data-tooltip-tip-value='#{order3.special_instructions}']" + expect(page).to have_content "Note" + find(".icon-warning-sign").hover + expect(page).to have_content /#{order3.special_instructions}/i end end end @@ -762,6 +763,11 @@ describe ' # checks shipment state expect(page).to have_content "READY" + + # move away from the Ship button so we can trigger the mouseenter event by moving back. + # We are already on the "Ship" button when it gets rendered because of + # the previous click + find(".icon-edit").hover # mouse-hovers and finds Ship tooltip find(".icon-road").hover expect(page).to have_content "Ship" From d2952d46a658a8b268b8a4a2a73862967062e9e6 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 25 Sep 2023 15:11:17 +0200 Subject: [PATCH 7/7] Add the highlighted version of the "ship" icon --- app/webpacker/css/admin/shared/tables.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/webpacker/css/admin/shared/tables.scss b/app/webpacker/css/admin/shared/tables.scss index e1f5abe35f..7346efff7c 100644 --- a/app/webpacker/css/admin/shared/tables.scss +++ b/app/webpacker/css/admin/shared/tables.scss @@ -84,7 +84,7 @@ table { background-color: $color-notice; color: $color-1; } - .icon-edit:hover, .icon-capture:hover, .icon-ok:hover, .icon-plus:hover { + .icon-edit:hover, .icon-capture:hover, .icon-ok:hover, .icon-plus:hover, .icon-road:hover { background-color: $color-success; color: $color-1; }