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

Site Settings: Use an info notices when changing the admin style and global edge cache #95546

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

matt-west
Copy link
Contributor

@matt-west matt-west commented Oct 21, 2024

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

Proposed Changes

  • Use an infoNotice in a loading state when changing the admin style or toggle global edge cache.

Before:

Screenshot 2024-10-21 at 11 32 17

After:

Screen.Recording.2024-10-21.at.15.05.43.mov

Why are these changes being made?

  • So that we’re displaying the blue banner that is used for informational notices.

Testing Instructions

Admin style

  • Ensure your site is using the Default admin style.
  • Go to Settings → General.
  • Change the admin style to Classic style and save.
  • The notice shown in the top right corner should have a blue background behind the icon.

Global edge cache

  • Go to Server Settings
  • Switch the Global edge cache toggle
  • Note that an info notice is displayed in the loading state

@matt-west matt-west changed the title Site Settings: Use an info notice when changing the admin style. Site Settings: Use an info notice when changing the admin style Oct 21, 2024
@matt-west matt-west marked this pull request as ready for review October 21, 2024 10:39
@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 Oct 21, 2024
@matticbot
Copy link
Contributor

matticbot commented Oct 21, 2024

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/changing-admin-style-notice on your sandbox.

@matticbot
Copy link
Contributor

matticbot commented Oct 21, 2024

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

Sections (~31 bytes added 📈 [gzipped])

name          parsed_size           gzip_size
staging-site        +54 B  (+0.0%)      +19 B  (+0.0%)
hosting             +54 B  (+0.0%)      +19 B  (+0.0%)
settings            +25 B  (+0.0%)      +12 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~18 bytes added 📈 [gzipped])

name                                       parsed_size           gzip_size
async-load-calypso-layout-command-palette        +29 B  (+0.0%)      +18 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

Copy link
Contributor

@Aurorum Aurorum left a comment

Choose a reason for hiding this comment

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

Code is all good, but one observation! I’m not sure it really makes sense to use an infoNotice for a busy state here. From what I can tell, it looks like Calypso favours plainNotice for this sort of thing. For example, the login screen uses plainNotice when sending an SMS etc.

(I know that there might be a reason for this change though - just a slight observation that it might be good to harmonise the use cases here)

@matt-west
Copy link
Contributor Author

Thanks for the feedback @Aurorum. We’d like to update these notices to use the info state so that they’re more visible to the user. We use blue elsewhere to convey progress (progress bars, spinners, etc.) so it makes sense to bring that consistency to these notices.

I’ll run through and see which other notices need updating.

Looking again at the dev docs, I discovered that the Notice component actually supports a loading state. I’ve enabled that here so that the icon background pulses.

Screen.Recording.2024-10-21.at.15.05.43.mov

@matt-west matt-west changed the title Site Settings: Use an info notice when changing the admin style Site Settings: Use an info notices when changing the admin style and global edge cache Oct 21, 2024
@matt-west matt-west self-assigned this Oct 22, 2024
@matt-west matt-west added [Feature] Site Settings All other general site settings. [Feature Group] Site Settings & Tools Settings and tools for managing and configuring your site. labels Oct 23, 2024
Copy link
Contributor

@fredrikekelund fredrikekelund left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Nice initiative. The loading state is a great touch.

I'm not concerned about a convention around plainNotice or infoNotice. The difference is all styling anyway. Thanks for the review, though, @Aurorum!

@matt-west matt-west removed the request for review from arthur791004 October 23, 2024 13:27
@matt-west matt-west merged commit 7d3bc49 into trunk Oct 23, 2024
14 of 15 checks passed
@matt-west matt-west deleted the fix/changing-admin-style-notice branch October 23, 2024 13:28
@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 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Site Settings & Tools Settings and tools for managing and configuring your site. [Feature] Site Settings All other general site settings.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants