[Admin] Add modal component #5364

the-krg · 2023-08-31T06:25:24Z

Summary

Those slots allow the developer to pass a block to the component, this enables an in-depth customization of the component, especially for the modal, in which the body should be dynamic. Since modals also carry action confirmations,
I've added the possibility to change the action button as well.

The modal is actually just a UI trick to show regular content and the close button (along with clicking on the backdrop) are simple links and just point to a URL which will need to be provided explicitly.

Currently we don't have a use case for alert() / confirm() style modals that are transactional, but we'll bake in that functionality if we'll bump into such cases.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 I have updated the README to account for my changes.
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

elia

Great! Left a couple of suggestions / questions 👍

elia · 2023-09-06T08:19:05Z

admin/app/components/solidus_admin/ui/modal/component.js

+ static values = { modal: String }
+
+ toggleModal(e) {
+ document.getElementById(e.currentTarget.dataset.modalId).classList.toggle('hidden');


Did you consider using action params already? I know they're way more verbose but would cleanup the JS a little bit. https://stimulus.hotwired.dev/reference/actions#action-parameters

Also if I understand this correctly in the presence of multiple modals in the same page the code would be executed by each of them and potentially multiple toggles could happen.

Nit: we could use the hidden attribute which is more semantic than the class, all browsers have a default stile for elements with it to be [hidden] {display:none} and we added a couple of TW variants to deal with it in classes:

plugin(({ addVariant }) => addVariant('hidden', '&([hidden])')), plugin(({ addVariant }) => addVariant('visible', '&:not([hidden])')),

Whoa, great insights! 🚀

I decided to stop using the hidden attribute and go with the open one, provided by the <dialog> element :)
Those variants are great tho!

WDYT?

Also if I understand this correctly in the presence of multiple modals in the same page the code would be executed by each of them and potentially multiple toggles could happen.

Hmmm 🤔 I don't think I understood. The target is the modal's id, so it wound only trigger the one that is on the data-ui--modal-modal-id-param attribute.
Also on the Lookbok's Overview I render 3 different modals on the same page. They all are correctly targeted.

admin/app/components/solidus_admin/ui/modal/component.html.erb

rainerdema

Overall, great work! 👏 I've left a few comments 👍

admin/app/components/solidus_admin/ui/modal/component.rb

rainerdema · 2023-09-11T13:11:19Z

admin/app/components/solidus_admin/ui/modal/component.html.erb

+ <button type="button" class="close" data-action=<%= "#{stimulus_id}#toggleModal" %> aria-label="Close" <%= "data-#{stimulus_id}-modal-id-param=#{@id}" %>>
+ <span aria-hidden="true">&times;</span>
+ </button>


Why not use a component('ui/button') for this button?

Unfortunately our button component only allows primary, secondary and ghost "layouts"; the x button on the modal does not match any of those.

elia

Great! I left some more notes, but looks good generally, up to you on how to act on each of them.

In any case some stuff will be adjusted once we start using it!

admin/app/components/solidus_admin/ui/modal/component.html.erb

admin/app/components/solidus_admin/ui/modal/component.rb

elia · 2023-09-11T13:17:06Z

admin/spec/components/previews/solidus_admin/ui/modal/component_preview/overview.html.erb

+ 'data-action': 'click->ui--modal#toggleModal',
+ 'data-controller': 'ui--modal',


We should find a way to avoid reusing the same controller for both the modal and the button opening it, but I think we can fix it later when we start using them.

rainerdema

👏

Those slots allow the developer to pass a block to the component, this enables an in-depth customization of the component, especially for the modal, in which the body should be dynamic. Co-Authored-By: Elia Schito <[email protected]>

rainerdema

💯

the-krg self-assigned this Aug 31, 2023

github-actions bot added the changelog:solidus_admin label Aug 31, 2023

the-krg changed the title ~~Add modal component~~ [Admin] Add modal component Aug 31, 2023

the-krg force-pushed the the-krg/admin/modal_component branch 3 times, most recently from b58519a to 5b7ec4e Compare September 5, 2023 20:56

the-krg marked this pull request as ready for review September 5, 2023 20:56

the-krg requested a review from a team as a code owner September 5, 2023 20:56

the-krg requested review from elia and rainerdema September 5, 2023 20:57

the-krg force-pushed the the-krg/admin/modal_component branch from 5b7ec4e to e42aacf Compare September 6, 2023 06:27

elia reviewed Sep 6, 2023

View reviewed changes

the-krg force-pushed the the-krg/admin/modal_component branch from e42aacf to 184b63c Compare September 7, 2023 06:05

the-krg requested a review from elia September 7, 2023 06:05

the-krg force-pushed the the-krg/admin/modal_component branch from 184b63c to cffd048 Compare September 7, 2023 06:22

rainerdema reviewed Sep 11, 2023

View reviewed changes

elia approved these changes Sep 11, 2023

View reviewed changes

the-krg force-pushed the the-krg/admin/modal_component branch from cffd048 to 2324e2f Compare September 12, 2023 06:05

the-krg requested a review from rainerdema September 12, 2023 06:29

rainerdema approved these changes Sep 13, 2023

View reviewed changes

elia force-pushed the nebulab/admin branch 5 times, most recently from 0a42f67 to 1288f9c Compare September 29, 2023 10:37

rainerdema force-pushed the the-krg/admin/modal_component branch from 2324e2f to edfa2d4 Compare September 29, 2023 16:52

rainerdema changed the base branch from nebulab/admin to main September 29, 2023 16:52

github-actions bot added changelog:solidus_backend Changes to the solidus_backend gem changelog:repository Changes to the repository not within any gem labels Sep 29, 2023

rainerdema added the hold On hold for a reason different from a breaking change label Sep 29, 2023

rainerdema force-pushed the the-krg/admin/modal_component branch from edfa2d4 to ce5fdec Compare September 29, 2023 16:55

github-actions bot added changelog:solidus_core Changes to the solidus_core gem and removed changelog:solidus_backend Changes to the solidus_backend gem labels Sep 29, 2023

rainerdema force-pushed the the-krg/admin/modal_component branch from ce5fdec to be002cc Compare September 29, 2023 16:55

github-actions bot removed changelog:solidus_core Changes to the solidus_core gem changelog:repository Changes to the repository not within any gem labels Sep 29, 2023

elia assigned elia and unassigned the-krg Oct 30, 2023

elia force-pushed the the-krg/admin/modal_component branch from be002cc to 66a61cf Compare October 30, 2023 17:51

elia requested a review from rainerdema October 30, 2023 17:51

Add modal component

8166fb7

Those slots allow the developer to pass a block to the component, this enables an in-depth customization of the component, especially for the modal, in which the body should be dynamic. Co-Authored-By: Elia Schito <[email protected]>

elia force-pushed the the-krg/admin/modal_component branch from 66a61cf to 8166fb7 Compare October 30, 2023 18:03

elia removed the hold On hold for a reason different from a breaking change label Oct 31, 2023

rainerdema approved these changes Oct 31, 2023

View reviewed changes

elia merged commit 8e9813a into solidusio:main Oct 31, 2023
9 checks passed

elia deleted the the-krg/admin/modal_component branch October 31, 2023 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Admin] Add modal component #5364

[Admin] Add modal component #5364

the-krg commented Aug 31, 2023 •

edited by elia

Loading

elia left a comment

elia Sep 6, 2023

elia Sep 6, 2023

elia Sep 6, 2023

the-krg Sep 7, 2023 •

edited

Loading

the-krg Sep 7, 2023

rainerdema left a comment

rainerdema Sep 11, 2023

the-krg Sep 12, 2023

elia left a comment

elia Sep 11, 2023

rainerdema left a comment

rainerdema left a comment

		'data-action': 'click->ui--modal#toggleModal',
		'data-controller': 'ui--modal',

[Admin] Add modal component #5364

[Admin] Add modal component #5364

Conversation

the-krg commented Aug 31, 2023 • edited by elia Loading

Summary

Checklist

elia left a comment

Choose a reason for hiding this comment

elia Sep 6, 2023

Choose a reason for hiding this comment

elia Sep 6, 2023

Choose a reason for hiding this comment

elia Sep 6, 2023

Choose a reason for hiding this comment

the-krg Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

the-krg Sep 7, 2023

Choose a reason for hiding this comment

rainerdema left a comment

Choose a reason for hiding this comment

rainerdema Sep 11, 2023

Choose a reason for hiding this comment

the-krg Sep 12, 2023

Choose a reason for hiding this comment

elia left a comment

Choose a reason for hiding this comment

elia Sep 11, 2023

Choose a reason for hiding this comment

rainerdema left a comment

Choose a reason for hiding this comment

rainerdema left a comment

Choose a reason for hiding this comment

the-krg commented Aug 31, 2023 •

edited by elia

Loading

the-krg Sep 7, 2023 •

edited

Loading