[#2253] Add new modal with buttons #1009

jiromaykin · 2024-02-08T08:42:50Z

~~issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2080~~
issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2253
old issue number: 2080 // new issue number: 2253.
Design: https://www.figma.com/file/iKGhWhstaLIlFSaND2q7cE/OIP---Designs-(new)?type=design&node-id=253%3A5932&mode=design&t=Emt5JXGEXJyBppaB-1

Notes to reviewers:
The use of comments in the javascript is intentional ;-)

Main issue:
is that we need to be able to set more options for all the different types of modals (The Modal constructor acts a bit like an 'interface' here because it is called multiple times in different ways from the Master template):
(We have modals with or without top-right Close/Cross button and w./without icons,
also: some modals only use the Confirmation button, or only the Cancel button, and some of them both...
if buttons are not used, they need to be invisible or else they become 'Tabbable' = bad for accessibility).

Different types of modals can be found on:
http://localhost:8000/mijn-profiel/ (2X)
http://localhost:8000/mijn-profiel/contacts/ (remove contact)
http://localhost:8000/samenwerken/create/ (plan-preview)
http://localhost:8000/mijn-profiel/actions/ (delete actions)
+sitewide sessionmodal
+sitewide accessibilit header modal

codecov-commenter · 2024-02-19T10:05:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.04%. Comparing base (f29e0af) to head (353d68d).
Report is 9 commits behind head on develop.

❗ Current head 353d68d differs from pull request most recent head c9e49cd. Consider uploading reports for the commit c9e49cd to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1009      +/-   ##
===========================================
+ Coverage    95.01%   95.04%   +0.02%     
===========================================
  Files          913      923      +10     
  Lines        32126    32343     +217     
===========================================
+ Hits         30525    30740     +215     
- Misses        1601     1603       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pi-sigma

Minor comments/questions

src/open_inwoner/components/templates/components/Modal/Modal.html

src/open_inwoner/js/components/accessibility/help_modal.js

pi-sigma · 2024-03-26T10:44:16Z

src/open_inwoner/js/components/confirmation/index.js

@@ -11,15 +13,21 @@ class Confirmation {
    if (!this.real_submit) {


real_submit is a strange name. Is there any other kind of submit field? I know it's not your change, but perhaps consider changing this (unless it makes sense to you and I just misunderstand).

Yeah that's not mine 🙂 but I understand why they chose this name: this name is often used when pop-up modals pop up after clicking something on the pages that just LOOKS like a Submit button, but actually isn't - and this modal warns the user about the data they are about to submit/delete - so not until they actually click the confirm button in the modal, does the "real" POST happen. So I think I'd like to keep this name.

Ah, that makes sense!

pi-sigma · 2024-03-26T10:57:50Z

One more thing I just noticed: when you hover over Ja, verwijdern, the button moves a little to the top. The other button does not.

Bartvaderkin

Just some notes for consideration.

Bartvaderkin · 2024-03-26T14:28:17Z

src/open_inwoner/templates/pages/plans/create.html

-                                        {% spaceless %}
-                                            {% button text=_("Sluiten") extra_classes="modal__button modal__close" primary=True %}
-                                        {% endspaceless %}
+                                        <button class="button modal__button modal__close button--primary button-icon--primary" type="button" aria-label="Sluit"><span class="inner-text">Sluiten </span></button>


I note this change uses plain html while the one above it uses the template tag (but I have no opinion on it)

The one in the top above it is a standard cross icon that needs no visible text so can just use the template - but the one below is where things got complicated: the original Modal was overwriting all buttons entirely, instead of just overwriting their inner-most text, which meant it was impossible to give Modal submit-buttons etc. an icon; so that is one of the things this wall of a PR is tackling.

Bartvaderkin · 2024-03-26T14:29:27Z

src/open_inwoner/templates/pages/profile/me.html

@@ -281,13 +281,12 @@ <h2 class="h2 title" id="profile-remove">{% trans "Profiel verwijderen" %}</h2><
                    {% trans "Hiermee worden alleen uw persoonlijke voorkeuren verwijderd. U krijgt dan bijvoorbeeld geen e-mail meer van ons over wijzigingen van uw lopende zaken. Uw persoonsgegevens en uw lopende zaken zelf worden hiermee niet verwijderd." %}
                </p>
            </div>
-            {% render_form form=form method="POST" id="delete-form" extra_classes="confirm" spaceless=True data_confirm_title=_("Weet u zeker dat u uw account wilt verwijderen?") data_confirm_text=_("Hiermee worden alleen uw persoonlijke voorkeuren verwijderd. U krijgt dan bijvoorbeeld geen e-mail meer van ons over wijzigingen van uw lopende zaken. Uw persoonsgegevens en uw lopende zaken zelf worden hiermee niet verwijderd.") data_confirm_cancel=_("Nee, annuleren") data_confirm_default=_("Ja, verwijderen") %}
+            {% render_form form=form method="POST" id="delete-form" extra_classes="confirm" spaceless=True data_confirm_title=_("Weet je zeker dat je jouw profiel wilt verwijderen?") data_confirm_text=_("Hiermee worden alleen uw persoonlijke voorkeuren verwijderd. U krijgt dan bijvoorbeeld geen e-mail meer van ons over wijzigingen van uw lopende zaken. Uw persoonsgegevens en uw lopende zaken zelf worden hiermee niet verwijderd.") data_confirm_cancel=_("Nee, annuleren") data_confirm_default=_("Ja, verwijderen") %}


Nice attribute wall 🙈

Bartvaderkin · 2024-03-26T14:57:09Z

src/open_inwoner/js/components/modal/index.js

+  setModalIcons(modalIcons) {
+    // Whether the modal-buttons should have icons or not
+    if (modalIcons) {
+      this.node.classList.add('show-modal-icons')
+    }
+  }


I note these functions only handle the truthy case (eg: don't remove/unset).

Yeah this had terrible and sad, lingering effects, so I changed that in the latest commit.

jiromaykin · 2024-03-26T15:41:51Z

One more thing I just noticed: when you hover over Ja, verwijdern, the button moves a little to the top. The other button does not.

@pi-sigma I see, it's the buttons that received :focus where this was happening + I also removed the jumpy motion from the close-cross because I find it annoying + I needed to adjust for the situation where the "session" modals do not overwrite other modals properly (like when a user has not closed a modal, it doesn't get overwritten by the end-session warning). So that's what I changed in the new commit.

pi-sigma · 2024-03-29T14:02:19Z

src/open_inwoner/scss/components/modal/_modal.scss

+
+      &:hover {
+        transform: translateY(-1px);
+      }


This has a strange effect: the button not only moves to the top but is also contracted (it looses one pixel on the right).

Yeah this is Firefox specific (Edge/Safari/Chrome do not have this). We also have this behaviour for ALL types of buttons that are hovered (like in the registration form etc.) so we'll promote this to a separate issue, since it's unrelated to the modals, so we can finalize the modals here :-)

In case someone wants to see the new separate issue: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2293

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 2 times, most recently from a25a71a to b6f430e Compare February 19, 2024 09:32

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 2 times, most recently from b344c94 to 465421b Compare February 20, 2024 11:30

jiromaykin added the wip Work in progress label Feb 22, 2024

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 4 times, most recently from f7ee948 to bb9ddc4 Compare February 29, 2024 18:20

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 3 times, most recently from a30dadf to b9c5131 Compare March 12, 2024 13:06

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 3 times, most recently from bc2b3c0 to c4fdcc0 Compare March 19, 2024 10:46

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch from 6bd97b3 to a011f09 Compare March 25, 2024 07:29

jiromaykin changed the title ~~[#2080] Add new modal with buttons~~ [#2253] Add new modal with buttons Mar 25, 2024

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 2 times, most recently from a0af972 to 09f4ac8 Compare March 25, 2024 11:25

jiromaykin removed the wip Work in progress label Mar 25, 2024

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch 7 times, most recently from 2706199 to c3d94fe Compare March 26, 2024 08:42

jiromaykin marked this pull request as ready for review March 26, 2024 08:44

jiromaykin requested a review from pi-sigma March 26, 2024 09:55

jiromaykin requested a review from Bartvaderkin March 26, 2024 09:55

pi-sigma requested changes Mar 26, 2024

View reviewed changes

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch from dc173e5 to c9e49cd Compare March 26, 2024 14:57

Bartvaderkin approved these changes Mar 26, 2024

View reviewed changes

jiromaykin requested a review from pi-sigma March 28, 2024 08:56

pi-sigma requested changes Mar 29, 2024

View reviewed changes

jiromaykin added 2 commits April 2, 2024 09:47

💄 [#2253] Added modal props and variations

b92e5a6

[#2253] Improved modals after PR feedback

9685a2b

jiromaykin force-pushed the feature/2080-new-design-modal-buttons branch from c9e49cd to 9685a2b Compare April 2, 2024 07:47

jiromaykin requested a review from pi-sigma April 2, 2024 08:29

pi-sigma approved these changes Apr 2, 2024

View reviewed changes

alextreme merged commit 79db037 into develop Apr 4, 2024
15 checks passed

alextreme deleted the feature/2080-new-design-modal-buttons branch April 4, 2024 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2253] Add new modal with buttons #1009

[#2253] Add new modal with buttons #1009

jiromaykin commented Feb 8, 2024 •

edited

Loading

codecov-commenter commented Feb 19, 2024 •

edited

Loading

pi-sigma left a comment

pi-sigma Mar 26, 2024

jiromaykin Mar 26, 2024

pi-sigma Mar 26, 2024

pi-sigma commented Mar 26, 2024

Bartvaderkin left a comment

Bartvaderkin Mar 26, 2024

jiromaykin Mar 26, 2024

Bartvaderkin Mar 26, 2024

Bartvaderkin Mar 26, 2024

jiromaykin Mar 26, 2024

jiromaykin commented Mar 26, 2024

pi-sigma Mar 29, 2024

jiromaykin Apr 2, 2024

jiromaykin Apr 2, 2024

		@@ -11,15 +13,21 @@ class Confirmation {
		if (!this.real_submit) {

[#2253] Add new modal with buttons #1009

[#2253] Add new modal with buttons #1009

Conversation

jiromaykin commented Feb 8, 2024 • edited Loading

codecov-commenter commented Feb 19, 2024 • edited Loading

Codecov Report

pi-sigma left a 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

pi-sigma commented Mar 26, 2024

Bartvaderkin left a 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

jiromaykin commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiromaykin commented Feb 8, 2024 •

edited

Loading

codecov-commenter commented Feb 19, 2024 •

edited

Loading