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: Add notice to newsletter onboarding #68841

Merged
merged 6 commits into from
Oct 13, 2022

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Oct 10, 2022

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

Proposed Changes

Screen Shot 2022-10-13 at 13 56 42

  • Add a notice to the newsletter onboarding to highlight that a color change during onboarding is a paid feature and requires upgrading for those changes to be made public.
  • Also add the limit-global-styles feature flag to the wpcalyps.json config file so it's easier to test in that environment.

Further reading: paYJgx-2p8-p2

Testing Instructions

  • Use the Calypso live link below
  • Go to /setup/newsletterSetup?flow=newsletter
  • Make sure the favorite color input displays a premium badge
  • Hover the badge
  • Make sure a notice shows up
  • Add a ?flags=-limit-global-styles param to the URL to disable the feature flag
  • Make sure the premium badge doesn't show up

@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Global Styles The Global Styles tools in the site editor and theme style variations. labels Oct 10, 2022
@mmtr mmtr requested review from SaxonF and a team October 10, 2022 15:45
@mmtr mmtr self-assigned this Oct 10, 2022
@matticbot
Copy link
Contributor

matticbot commented Oct 10, 2022

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~445 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
entry-stepper      +1525 B  (+0.0%)     +445 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

{ isEnabled( 'limit-global-styles' ) && (
<Tooltip
text={ __(
'Upgrade to a paid plan for color changes to take affect and to unlock all premium design tools'
Copy link
Member Author

Choose a reason for hiding this comment

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

We might want to replace "premium design tools" here with whatever feature name we finally give to Global Styles. See pdgrnI-1Ix-p2

@BogdanUngureanu
Copy link
Contributor

hey @mmtr, looks nice, great work! I've noticed some small issues while on mobile:

  • I've tested this on my phone and I've noticed that the pop-up is not displayed because the colorpicker takes control when I tap the label. On desktop, when I click on the label, the colorpicker takes control as well.
  • On desktop, on small resolutions, the pop-up div is not responsive and adds a vertical scroll when you hover it.

@mmtr
Copy link
Member Author

mmtr commented Oct 12, 2022

I've tested this on my phone and I've noticed that the pop-up is not displayed because the colorpicker takes control when I tap the label. On desktop, when I click on the label, the colorpicker takes control as well.

Good catch, @BogdanUngureanu.

@SaxonF do you have any ideas on how to prevent this? One thing that could avoid the issue is to display the tooltip on click, but I don't think it's immediately obvious that the badge is clickable.

@mmtr
Copy link
Member Author

mmtr commented Oct 12, 2022

do you have any ideas on how to prevent this?

Finally found out a way in 75d5e21 so the tooltip shows up when hovering on desktop, and when clicking/tapping on mobile.

@BogdanUngureanu
Copy link
Contributor

Looks and the interactions are way better now, thanks for implementing the suggestions.

There's one small thing that maybe we should also implement: On mobile, when the popover is opened, it's not automatically closed when the user taps on other elements than the "Premium" badge.

@dsas
Copy link
Contributor

dsas commented Oct 13, 2022

it's not automatically closed when the user taps on other elements than the "Premium" badge.

It'd be great if tapping on the tooltip dismissed it rather than launching the colour picker.

@dsas
Copy link
Contributor

dsas commented Oct 13, 2022

Actually it looks like onClickOut

onClickout = ( event ) => {
should do dismiss when clicking outside but isn't working?

@mmtr
Copy link
Member Author

mmtr commented Oct 13, 2022

@dsas that's the Calypso component, not the Gutenberg one which is the one I decided to use. But I'll probably switch to the Calypso component if it works better.

@mmtr
Copy link
Member Author

mmtr commented Oct 13, 2022

The Popover component from Calypso works great! The tooltip is automatically closed now when clicking/tapping outside.

Copy link
Contributor

@BogdanUngureanu BogdanUngureanu left a comment

Choose a reason for hiding this comment

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

Works great on both mobile and desktop! 🎉

@mmtr mmtr merged commit c3c3ea6 into trunk Oct 13, 2022
@mmtr mmtr deleted the add/newsletter-setup-color-premium branch October 13, 2022 16:56
@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 Oct 13, 2022
@a8ci18n
Copy link

a8ci18n commented Oct 13, 2022

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

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

@a8ci18n
Copy link

a8ci18n commented Oct 21, 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
[Feature] Global Styles The Global Styles tools in the site editor and theme style variations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants