Rewrite confirmation modals #1985

niklasmohrin · 2023-07-17T20:40:35Z

Our modals have been bugging me for a while now and I believe the number of times I announced that I would rewrite them is just out of finger range. I am not sure how exactly it will look in the end, but I want it to be less Django magic and more browser native. My first vision here uses a custom web component at the use-site. This allows the logic of the modal to be together with the opening button. Furthermore, everything about the modal is contained in one HTML tag.

In my current plan, there won't be multiple users of one modal anymore, but instead multiple copies of the same template (specified by the custom web component). This will keep HTML size similar to what we had before, but increase DOM size significantly on sites with many modals (for example lists with deletable entries). We will have to see the effect this has on performance, I would believe that modern browsers should be able to handle this, but you never know. If it turns out to be slow, I would divert to trying to implement some kind of reference-modal component for sharing DOM nodes.

Thoughts?

richardebeling

Looking good! I like the declarative way to defining modals. The whole ElementInternals API is new to me, I can't really tell if the code is canonical, but it seems reasonable to me. It would still be nice to have frontend tests for that -- maybe one for a modal that can be directly submitted and one for semester deletion with the input validation?

evap/static/scss/components/_confirmation-modal.scss

richardebeling · 2023-11-21T16:23:36Z

(when converting new modals, it might make sense to do a separate commit for each modal so that reviewing becomes easier -- moved code is always hard to review)

niklasmohrin · 2024-01-08T19:20:05Z

I decided to leave the couple of contact modals as they are for now, because this PR is already huge

evap/grades/templates/grades_course_view.html

evap/staff/templates/staff_semester_preparation_reminder.html

evap/static/ts/src/confirmation-modal.ts

evap/staff/templates/staff_evaluation_person_management.html

evap/staff/templates/staff_semester_view.html

evap/grades/templates/grades_course_view.html

evap/rewards/templates/rewards_reward_point_redemption_events.html

evap/contributor/templates/contributor_index.html

evap/static/scss/components/_confirmation-modal.scss

janno42

Please change placing to a central position
The focus should probably not be on the close button. Maybe on "Cancel"? Or in case of an additional form field on that field?

evap/contributor/templates/contributor_evaluation_form.html

evap/evaluation/templates/confirmation_modal_template.html

evap/contributor/templates/contributor_index.html

evap/rewards/templates/rewards_reward_point_redemption_event_list.html

evap/staff/templates/staff_questionnaire_index_list.html

evap/staff/templates/staff_semester_view.html

evap/staff/templates/staff_user_form.html

richardebeling

Code is looking good

(Haven't tested. There is a chance that I may not spot errors in HTML changes.)

evap/grades/templates/grades_course_view.html

evap/staff/templates/staff_semester_view.html

.

More realistic alternative to `current` Add custom elements scaffold looks right, doesn't open found a way to use import, but not have icons modal texts displayed before script is loaded fix usage of custom elements prototype before it was loaded make it open the modal dont know some progess with native dialogs make it look somewhat like bootstrap progress? make animations work implement confirmation logic ough, now the slots inherit values from the .card-header bar Use `:defined` instead of `.loaded` class more modals and fix blocktrans Allow putting extra stuff under `confirmation-modal` more modals format more modals

Co-authored-by: Johannes Wolf <[email protected]>

…ot broken

…on an `<a>` ancestor, even if the dialog stops event propagation in a handler

…tion

richardebeling

Code is nice, all modals work for me except for the delegation modal

evap/static/scss/components/_confirmation-modal.scss

evap/contributor/templates/contributor_index.html

evap/staff/templates/staff_semester_view.html

richardebeling

Thanks! We can merge if/when remaining discussions are resolved.

niklasmohrin · 2024-02-26T19:42:09Z

Should I try to make a handful of useful commits out of this (not today), or should we just squash it?

richardebeling · 2024-02-26T19:43:47Z

I'm fine with just squashing

Kakadus

Tick tack...

richardebeling · 2024-02-26T20:04:28Z

Should we create a follow-up issue for the contact modal and the delegate preparation modal?

niklasmohrin · 2024-02-26T20:06:22Z

Sounds good!

niklasmohrin force-pushed the rewrite-modals branch 2 times, most recently from dd3cedd to 943c7ce Compare July 17, 2023 21:30

niklasmohrin force-pushed the rewrite-modals branch from 1068fc5 to a79e7b3 Compare September 3, 2023 17:44

niklasmohrin force-pushed the rewrite-modals branch from ae36e95 to 6edafce Compare September 25, 2023 16:47

niklasmohrin force-pushed the rewrite-modals branch 3 times, most recently from b94211d to a219e47 Compare November 13, 2023 21:09

niklasmohrin requested review from richardebeling and Kakadus November 20, 2023 18:51

richardebeling reviewed Nov 21, 2023

View reviewed changes

evap/static/scss/components/_confirmation-modal.scss Outdated Show resolved Hide resolved

niklasmohrin force-pushed the rewrite-modals branch from 407d4a4 to 3253038 Compare December 11, 2023 20:50

niklasmohrin changed the title ~~Rewrite modals~~ Rewrite confirmation modals Jan 8, 2024

niklasmohrin marked this pull request as ready for review January 8, 2024 19:20

niklasmohrin requested review from richardebeling and janno42 January 8, 2024 19:20

richardebeling mentioned this pull request Jan 8, 2024

[waiting for django5.1] Content Security Policy #2099

Closed

4 tasks

richardebeling previously requested changes Jan 16, 2024

View reviewed changes

niklasmohrin force-pushed the rewrite-modals branch 2 times, most recently from 0dcf19f to 04a8dda Compare January 22, 2024 20:05

niklasmohrin requested a review from richardebeling January 22, 2024 20:06

Kakadus reviewed Jan 22, 2024

View reviewed changes

evap/contributor/templates/contributor_index.html Outdated Show resolved Hide resolved

Kakadus reviewed Jan 22, 2024

View reviewed changes

evap/static/scss/components/_confirmation-modal.scss Outdated Show resolved Hide resolved

janno42 requested changes Jan 29, 2024

View reviewed changes

richardebeling mentioned this pull request Feb 5, 2024

Use translate and blocktranslate template tags #2114

Merged

niklasmohrin mentioned this pull request Feb 12, 2024

Use click handlers instead of inserting anchor tags in .table-seamless-links #2117

Merged

richardebeling reviewed Feb 15, 2024

View reviewed changes

evap/grades/templates/grades_course_view.html Outdated Show resolved Hide resolved

evap/grades/templates/grades_course_view.html Show resolved Hide resolved

evap/staff/templates/staff_semester_view.html Outdated Show resolved Hide resolved

niklasmohrin and others added 7 commits February 22, 2024 18:31

Apply suggestions from code review

13e274e

Co-authored-by: Johannes Wolf <[email protected]>

use blocktrans trimmed everywhere

7731b48

Fix semester archival buttons

1e454bb

Fix semester header margin

e052f16

Sync modal disabled state with show button for style reasons

eb0cce5

make semester name confirmation box in semester deletion modal look n…

00f5940

…ot broken

Fix bug in chrome where clicking with an open modal triggers a click …

a6bdd98

…on an `<a>` ancestor, even if the dialog stops event propagation in a handler

niklasmohrin force-pushed the rewrite-modals branch from 1cdb0a0 to a6bdd98 Compare February 22, 2024 17:33

niklasmohrin added 2 commits February 22, 2024 18:38

Use form= instead of wrapping table in form for grade document dele…

ad3663e

…tion

Fix leftover async script tags

c057e8d

niklasmohrin requested review from richardebeling, Kakadus and janno42 February 22, 2024 17:39

richardebeling requested changes Feb 23, 2024

View reviewed changes

niklasmohrin added 4 commits February 25, 2024 23:07

Remove dot

7804728

git checkout main -- evap/contributor/templates/contributor_index.html

1ad8c7e

Fixes for the old modal

216472f

./manage.py format

4dbbaec

richardebeling approved these changes Feb 26, 2024

View reviewed changes

janno42 approved these changes Feb 26, 2024

View reviewed changes

Kakadus approved these changes Feb 26, 2024

View reviewed changes

Merge branch 'main' into rewrite-modals

d1bcd08

niklasmohrin merged commit 04015c9 into e-valuation:main Feb 26, 2024
11 of 12 checks passed

niklasmohrin deleted the rewrite-modals branch February 26, 2024 20:02

richardebeling mentioned this pull request Feb 26, 2024

Use web components for contact and direct delegation modals #2129

Open

richardebeling mentioned this pull request Mar 25, 2024

"Delete Semester" (modal submit) button flicker-shows on staff semester page #2150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite confirmation modals #1985

Rewrite confirmation modals #1985

niklasmohrin commented Jul 17, 2023 •

edited

Loading

richardebeling left a comment

richardebeling commented Nov 21, 2023

niklasmohrin commented Jan 8, 2024

janno42 left a comment

richardebeling left a comment

richardebeling left a comment

richardebeling left a comment

niklasmohrin commented Feb 26, 2024 •

edited

Loading

richardebeling commented Feb 26, 2024

Kakadus left a comment

richardebeling commented Feb 26, 2024

niklasmohrin commented Feb 26, 2024

Rewrite confirmation modals #1985

Rewrite confirmation modals #1985

Conversation

niklasmohrin commented Jul 17, 2023 • edited Loading

richardebeling left a comment

Choose a reason for hiding this comment

richardebeling commented Nov 21, 2023

niklasmohrin commented Jan 8, 2024

janno42 left a comment

Choose a reason for hiding this comment

richardebeling left a comment

Choose a reason for hiding this comment

richardebeling left a comment

Choose a reason for hiding this comment

richardebeling left a comment

Choose a reason for hiding this comment

niklasmohrin commented Feb 26, 2024 • edited Loading

richardebeling commented Feb 26, 2024

Kakadus left a comment

Choose a reason for hiding this comment

richardebeling commented Feb 26, 2024

niklasmohrin commented Feb 26, 2024

niklasmohrin commented Jul 17, 2023 •

edited

Loading

niklasmohrin commented Feb 26, 2024 •

edited

Loading