Skip to content

DPMMA-2938 GrapesJS Text Editor Modal Save UX Improvements #14533

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 30, 2025
Merged

DPMMA-2938 GrapesJS Text Editor Modal Save UX Improvements #14533

merged 2 commits into from
Jan 30, 2025

Conversation

patrykgruszka
Copy link
Member

@patrykgruszka patrykgruszka commented Jan 30, 2025 •
edited
Loading

Q A
Bug fix? (use the a.b branch) 🔴
New feature/enhancement? (use the a.x branch) 🟢
Deprecations? 🔴
BC breaks? (use the c.x branch) 🔴
Automated tests included? 🔴
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

This PR introduces UX improvements to the GrapesJS text editor modal:

  • Clicking the background outside the modal no longer closes it.
  • The Close button name has been changed to Cancel

image

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create a new Email.
  3. Select a theme and open the builder.
  4. Double-click some text to open the text editor modal.
  5. Click outside the modal to verify that it does not close.
  6. Click the "Cancel" button to ensure no changes are applied and the modal closes.

@patrykgruszka patrykgruszka added enhancement Any improvement to an existing feature or functionality builder-grapesjs Anything related to the GrapesJS email or landing page builders T1 Low difficulty to fix (issue) or test (PR) labels Jan 30, 2025
andersonjeccel
andersonjeccel suggested changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@andersonjeccel andersonjeccel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying to get rid of having Save + Save and Close on the entire platform because this is an UX issue, so this might be something to remove from this PR to move it forward...

For modals, the only option must be "Save" (which will save & close), while static pages (like creating a segment) must use only "Apply" (will save without closing, without offering the option to Save & Close)

In the future, when you click to Apply changes, the Cancel button will dynamically update to "Close" to better reflect the action
While changes are present, it'll remain "Cancel" and present a confirmation message before discarding your in-progress work

So in this case, the fix would be to remove the "Apply" option and behavior from the modal, leaving only the "Save" option labelled exactly this way (no "Save & Close" or "Apply & Close")

@andersonjeccel andersonjeccel added the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 30, 2025
@patrykgruszka
Copy link
Member Author

@andersonjeccel makes sense. After your suggestion this PR introduces two changes:

  • Clicking the background outside the modal no longer closes it.
  • The Close button name has been changed to Cancel

@codecov Codecov
Copy link

codecov bot commented Jan 30, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.04%. Comparing base (8005ca1) to head (dc912d9).
Report is 7 commits behind head on 6.x.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                6.x   #14533      +/-   ##
============================================
- Coverage     64.04%   64.04%   -0.01%     
  Complexity    34652    34652              
============================================
  Files          2287     2287              
  Lines        103718   103718              
============================================
- Hits          66427    66426       -1     
- Misses        37291    37292       +1

see 1 file with indirect coverage changes

andersonjeccel
andersonjeccel approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@andersonjeccel andersonjeccel left a comment

Choose a reason for hiding this comment

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

It works as expected, thank you for fixing it!

image

When clicking outside, the modal keeps open
If selecting the text and dragging the cursor out of the modal, it also stays opened

Screencast.From.2025-01-30.10-27-10.mp4

@andersonjeccel andersonjeccel added code-review-needed PR's that require a code review before merging user-testing-passed PRs which have been successfully tested by the required number of people. ux-review-passed and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Jan 30, 2025
@andersonjeccel andersonjeccel added this to the 6.0.0-beta milestone Jan 30, 2025
escopecz
escopecz approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

This was very annoying behavior. I bet a lot of content was lost and had to be re-created again because of this. Thanks for taking the time! 👍

@escopecz escopecz added code-review-passed PRs which have passed code review and removed code-review-needed PR's that require a code review before merging labels Jan 30, 2025
@escopecz escopecz merged commit fd68ed0 into mautic:6.x Jan 30, 2025
17 checks passed
@biozshock biozshock mentioned this pull request Feb 6, 2025
Merged
@RCheesley RCheesley modified the milestones: 6.0.0-beta, 6.0.0-beta2 Mar 5, 2025
@mautibot
Copy link
Contributor

mautibot commented Mar 6, 2025

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/announcing-mautic-6-beta-now-available-for-testing/35196/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz approved these changes

+1 more reviewer

@andersonjeccel andersonjeccel andersonjeccel approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
builder-grapesjs Anything related to the GrapesJS email or landing page builders code-review-passed PRs which have passed code review enhancement Any improvement to an existing feature or functionality T1 Low difficulty to fix (issue) or test (PR) user-testing-passed PRs which have been successfully tested by the required number of people. ux-review-passed
Projects
None yet
Milestone
6.0.0-beta2
Development

Successfully merging this pull request may close these issues.
5 participants
@patrykgruszka @mautibot @escopecz @andersonjeccel @RCheesley