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

Limit Global Styles: Update the Pre-Save Notice #70525

Merged
merged 4 commits into from
Dec 1, 2022

Conversation

rcrdortiz
Copy link
Contributor

@rcrdortiz rcrdortiz commented Nov 29, 2022

Proposed Changes

Fixes: https://github.com/Automattic/dotcom-forge/issues/1221

  • Changed the copy of the pre-save notice.
  • Stop showing the reset link in the notice's message.
  • Refactor the observer that inserts the notice in the pre-save panel.
Before After
image Screenshot 2022-11-30 at 20 58 57

Testing Instructions

  • Setup:
    • Apply these changes to your sandbox: install-plugin.sh editing-toolkit update/gs-pre-save-panel-notice
    • Go to https://wordpress.com/start and create a new free site
    • Add the wpcom-limit-global-styles blog sticker to the new site: add_blog_sticker( 'wpcom-limit-global-styles', 'test', <YOUR_USERNAME>, <YOUR_BLOG_ID>).
    • Sandbox the new site.
  • Open the Site Editor.
  • Open the Global Styles sidebar.
  • ⚠️ (check for regression) Ensure the paid Global Styles modal appears.
  • Do some changes in the canvas (not on Global Styles just yet) and click "Save".
  • ⚠️ Ensure the Global Styles notice does not appear.
  • Do some changes to Global Styles and click "Save" again.
  • ⚠️ Ensure the Global Styles notice appears and is appended to the correct entity.
  • Try playing around with the Global Styles customizations and toggling the pre-save panel.
  • ⚠️ Ensure the Global Styles notice behaves as expected (appears and disappears when it should).

Pre-merge Checklist

  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

Related to #

@matticbot
Copy link
Contributor

matticbot commented Nov 29, 2022

@rcrdortiz rcrdortiz requested a review from a team November 29, 2022 12:27
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2022
@rcrdortiz rcrdortiz self-assigned this Nov 29, 2022
@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit update/gs-pre-save-panel-notice on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@rcrdortiz rcrdortiz changed the title Gating Global Styles: Changed the notice text and we also now suggest premium plan as default [WIP] Gating Global Styles: Changed the notice text and we also now suggest premium plan as default Nov 29, 2022
@rcrdortiz rcrdortiz assigned rcrdortiz and unassigned rcrdortiz Nov 29, 2022
@Copons Copons force-pushed the update/gs-pre-save-panel-notice branch from c73ffc5 to a5ad3ee Compare November 30, 2022 19:32
@Copons Copons changed the title [WIP] Gating Global Styles: Changed the notice text and we also now suggest premium plan as default Limit Global Styles: Update the Pre-Save Notice Nov 30, 2022
@Copons Copons self-assigned this Nov 30, 2022
Copy link
Contributor

@dsas dsas left a comment

Choose a reason for hiding this comment

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

This isn't working for me 🤔 The 'Are you ready to save' prompt displays but it does not display the warning about needing to upgrade for global styles. I do have the sticker enabled.

The console says

Uncaught TypeError: path is undefined
    trackGlobalStylesMenuSelected wpcom-block-editor-global-styles-menu-selected.js:18
    __WEBPACK_DEFAULT_EXPORT__ delegate-event-tracking.js:116
    __WEBPACK_DEFAULT_EXPORT__ delegate-event-tracking.js:111
    delegateCaptureListener tracking.js:865

@dsas
Copy link
Contributor

dsas commented Dec 1, 2022

Console message comes when switching to e.g. colours in the GS menu, so it's probably unrelated.

I get the same behaviour on blog 212978125 and 212976996 when testing via my sandbox.

Weirdly if I checkout out the branch locally and do a yarn dev --sync it does work 😕

Copy link
Contributor

@dsas dsas left a comment

Choose a reason for hiding this comment

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

Tried again after nuking my dev ETK and it works. I think earlier I had some dev ETK content on my sandbox and that was being used rather than the sandbox install (which goes to prod). sorry.

@Copons Copons merged commit c2e130b into trunk Dec 1, 2022
@Copons Copons deleted the update/gs-pre-save-panel-notice branch December 1, 2022 14:59
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 1, 2022
@a8ci18n
Copy link

a8ci18n commented Dec 1, 2022

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7650133

Thank you @rcrdortiz for including a screenshot in the description! This is really helpful for our translators.

@a8ci18n
Copy link

a8ci18n commented Dec 7, 2022

Translation for this Pull Request has now been finished.

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.

None yet

5 participants