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.

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

Add site setting to hide the Action Bar #98318

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Add site setting to hide the Action Bar #98318

wants to merge 4 commits into from
+44 −0

Conversation

ntsekouras
Copy link
Member

@ntsekouras ntsekouras commented Jan 14, 2025 •
edited
Loading

Related to #53830

This PR adds a new general setting (wpcom_hide_action_bar) to conditionally show the Action Bar on the front end of a site. The default value will be falsy to preserve the current behavior that Action Bar is shown.

The main issue will need 1 more PRs to be complete that I'll create and link shortly:

  1. 169996-ghe-Automattic/wpcom to add the check for this option and not enqueue the Action Bar. Additionally it handles the setting in classic wp-admin interface.
  2. Jetpack PR to update the settings REST endpoint for Calypso UI

Notes

  1. Some other options are being tracked. Should it be the case for this one too?

Testing Instructions

You need to test this PR with the other linked PRs.

  1. Go to general settings and update the value
  2. Go to the front-end and observe that the Action Bar is rendered based on that option.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • 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 UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • 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-aUh-p2)?

@ntsekouras ntsekouras added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Site Settings All other general site settings. [Feature] Action Bar labels Jan 14, 2025
@ntsekouras ntsekouras self-assigned this Jan 14, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 14, 2025 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-130151
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-130151&env=jetpack
Automattic for Agencies live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-130151&env=a8c-for-agencies

@matticbot
Copy link
Contributor

matticbot commented Jan 14, 2025 •
edited
Loading

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

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

  • blaze-dashboard
  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/show-action-bar-setting-ui on your sandbox.

@matticbot
Copy link
Contributor

matticbot commented Jan 14, 2025 •
edited
Loading

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

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

name                 parsed_size           gzip_size
entry-subscriptions        +22 B  (+0.0%)       +7 B  (+0.0%)
entry-stepper              +22 B  (+0.0%)       +7 B  (+0.0%)
entry-main                 +22 B  (+0.0%)       +7 B  (+0.0%)
entry-login                +22 B  (+0.0%)       +7 B  (+0.0%)

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

Sections (~231 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
settings            +713 B  (+0.1%)     +211 B  (+0.1%)
site-settings        +71 B  (+0.0%)      +20 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-context-links        +79 B  (+0.8%)      +18 B  (+0.7%)

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.

@ntsekouras ntsekouras force-pushed the add/show-action-bar-setting-ui branch from 3cf3d66 to 6ed16c3 Compare January 16, 2025 09:50
@ntsekouras ntsekouras mentioned this pull request Jan 16, 2025
Open
3 tasks
@ntsekouras ntsekouras requested review from mattwiebe and tyxla January 16, 2025 13:31
@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 Jan 16, 2025
tyxla
tyxla reviewed Jan 16, 2025
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This looks great, thank you for working on it @ntsekouras 🙌

Left some suggestions, let me know what you think.

client/my-sites/site-settings/form-general.jsx
@@ -2,6 +2,7 @@ import { isEnabled } from '@automattic/calypso-config';
import { Card, Button, FormLabel, Gridicon } from '@automattic/components';
import { guessTimezone, localizeUrl } from '@automattic/i18n-utils';
import languages from '@automattic/languages';
import { ToggleControl } from '@wordpress/components';
Copy link
Member

Choose a reason for hiding this comment

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

Nice to be able to use @wordpress/components directly!

client/my-sites/site-settings/form-general.jsx Outdated Show resolved Hide resolved
client/my-sites/site-settings/form-general.jsx
@@ -413,6 +439,7 @@ export class SiteSettingsFormGeneral extends Component {
{ this.blogAddress() }
{ this.languageOptions() }
{ this.Timezone() }
{ ! siteIsJetpack && this.hideActionbar() }
Copy link
Member

Choose a reason for hiding this comment

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

We're intentionally adding this one just for WP.com simple sites, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so yes, since Action Bar is not available to atomic sites. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I think so yes, since Action Bar is not available to atomic sites.

Right. Atomic sites are also Jetpack sites.

Am I missing something?

I don't think you're missing anything. I'm mostly making sure the intent is clearly communicated between the author, reviewer, and everyone else reading this later (including our future selves).

client/my-sites/site-settings/form-general.jsx Show resolved Hide resolved
client/my-sites/site-settings/form-general.jsx Outdated Show resolved Hide resolved
client/my-sites/site-settings/form-general.jsx Outdated
/>
<FormSettingExplanation>
<a href="https://en.support.wordpress.com/action-bar/">
{ translate( 'Learn more about the Action Bar.' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, check how this privacy link opens the help center. It would be nice if this one can do the same.

See a comparison of the behavior of those two links in this video:

Screen.Recording.2025-01-16.at.17.02.52.mov

Copy link
Member Author

@ntsekouras ntsekouras Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I made the suggested change but I'm not sure how to test it successfully. I tried different locales but it's very possible the specific page has no translations yet (or at least the languages I tried)? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I just checked, and you're right: that page has never been translated into other languages.

cc @Automattic/i18n and @dlind1 - do we want to translate it, or is it intentionally left out?

Copy link
Contributor

Choose a reason for hiding this comment

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

That wasn't intentional, but also not an oversight since not every page is automatically being translated :)
I've added it to the queue and it should be sent for translation on Monday the 20th.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks @dlind1!

@ntsekouras ntsekouras force-pushed the add/show-action-bar-setting-ui branch from 5798578 to 7a9a8da Compare January 17, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyxla tyxla tyxla left review comments

@dlind1 dlind1 dlind1 left review comments

@mattwiebe mattwiebe Awaiting requested review from mattwiebe

Assignees

@ntsekouras ntsekouras

Labels
[Feature] Action Bar [Feature] Site Settings All other general site settings. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ntsekouras @matticbot @dlind1 @tyxla