Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the real submit buttons to inside the dialogs #63

Merged
merged 5 commits into from
Aug 1, 2024

Conversation

mgax
Copy link
Member

@mgax mgax commented Jul 31, 2024

@mgax mgax requested a review from laymonage July 31, 2024 14:25
@mgax mgax marked this pull request as ready for review July 31, 2024 14:25
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the overall changes here! The subtitle dialog can be simplified by passing the message string to the {% dialog %} tag directly.

Glad to see that the dialog-root-selector helps with reducing the Stimulus targets.

@mgax
Copy link
Member Author

mgax commented Aug 1, 2024

@laymonage thank you for the naming and phrasing suggestions. My brain was fried after too much messing with events, CSS, templates and whatnot 😅

@mgax mgax requested a review from laymonage August 1, 2024 16:22
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the changes here! Would be nice to completely prevent the "send test email" submission if the email field is left empty, but I couldn't find a way to trigger the browser's "required" popup from my brief research. One small typo but I think you can fix that as you merge.

@mgax
Copy link
Member Author

mgax commented Aug 1, 2024

Would be nice to completely prevent the "send test email" submission if the email field is left empty, but I couldn't find a way to trigger the browser's "required" popup from my brief research.

That used to work, when the dialog had its own form, that only contained the email input. I'm not sure what's going on with Wagtail's page edit form.

@mgax mgax merged commit d3477de into main Aug 1, 2024
12 checks passed
@mgax mgax deleted the buttons-in-dialogs-in-form branch August 1, 2024 17:11
@laymonage laymonage self-assigned this Aug 2, 2024
@laymonage
Copy link
Member

That used to work, when the dialog had its own form, that only contained the email input. I'm not sure what's going on with Wagtail's page edit form.

Yeah it's because Wagtail's editor form has the novalidate attribute (reasons described in wagtail/wagtail#2942), so you'd have to temporarily remove the attribute while the test email dialog is shown (and to add it back again afterwards). A bit of trouble than it's worth for now, but may be something to look into in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
2 participants