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

Add an "Are you sure" interstitial #9

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Conversation

circlecube
Copy link
Member

@circlecube circlecube commented Mar 8, 2024

Proposed changes

This adds an "Are you sure?" interstitial before showing the deactivation survey.

I added styles - inspired by similar interstitials and updated the survey styles for consistency.

This is still waiting on better/final copy/content from @chrisdavidmiles but the code is set up now, I'll add some screenshots for preview.

Screenshot 2024-03-08 at 4 34 45 PM
Screenshot 2024-03-08 at 4 38 25 PM

TODO:

  • Still needs a hiive event added for advancing to the survey.
  • Add content strings to localized strings for translation and plugin overrides.
  • Update tests to reflect the new flow.

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@circlecube circlecube self-assigned this Mar 8, 2024
Comment on lines 35 to 43
<p class="nfd-deactivation__content-subtitle">This plugin is powers important features on your site. These will no longer be available if you deactivate the plugin.</p>
</div>
<div class="nfd-deactivation__body">
<div class="nfd-deactivation__cards">
<div class="nfd-deactivation__card">Hosting integrations</div>
<div class="nfd-deactivation__card">Performance Improvements</div>
<div class="nfd-deactivation__card">Staging Site Support</div>
<div class="nfd-deactivation__card">Wonder Blocks</div>
<div class="nfd-deactivation__card">Integrated AI Help Center</div>
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that we add the copy to the runtimeData.strings to match the rest of the module copy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added all content to the runtime data strings and set it up for overrides in the container from plugins.

<div class="nfd-deactivation__footer"><div class="nfd-deactivation__footer-content">
<div class="nfd-deactivation-survey__content-actions">
<div class="nfd-deactivation__helptext" "style="text-align:left">
<p><b>Need Help?</b> Learn more about <a href="#" target="_blank" nfd-deactivation-survey-destroy>the features of this plugin</a>, check the <a href="#">help center</a>, or <a href="#">contact support</a>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Same here with the copy, and the href doesn't seem to be going anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added placeholder urls, and will likely rework this section once we have copy.

</button>
<button type="button" nfd-deactivation-survey-next
class="button button-seconday"
aria-label="${ runtimeData.strings.submitAriaLabel }">
Copy link
Member

Choose a reason for hiding this comment

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

AriaLabel mismatch

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@wpalani
Copy link
Member

wpalani commented Mar 10, 2024

Another thing to add to the code review, I'm not super keen on the design, few things stood out to me:

  • Too much white space / padding around the modal content.
  • Inconsistent height between steps 1 and 2, Probably okay for now since the content is different.
  • Not sure we need to change the modal width from the initial design, I think we can fit it in the original width if we reduce the overall size.
  • The typography sizing is different in step 1 than step 2.
  • The main title and subtitle in step 2 are very close in size, which makes them competing in priority.

deactivation-step-1

deactivation-step-2

@circlecube
Copy link
Member Author

Yes @wpalani - I should have marked this as "not ready" since I'm waiting on some final copy. I am still planning on updating the strings to be pulled in from the runtime data and exposing that so plugins can override it. And other things like making sure the link goes somewhere )if we even want a link there). As content finalizes too I am planning another round of style tweaks. Loosely basing the design off the one in JP.

cards have title and description text and a conditional to be evaluated determining visibility of the card.
@circlecube
Copy link
Member Author

New preview screenshots:

screenshot with wo active

screenshot with woo inactive

screenshot deactivation survey

@circlecube circlecube merged commit 83447e2 into main Mar 18, 2024
6 checks passed
@circlecube circlecube deleted the add/are-you-sure-interstitial branch March 18, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants