Masterbar: Add a label for the Reader icon.

[spsiddarthan]

Related to pe7F0s-2rg-p2

Proposed Changes

• Add a label for the Reader icon

Why are these changes being made?

• The traffic to the Reader has reduced since we switched to the glasses icon. We are experimenting to see if adding a label increases the traffic.

Testing Instructions

• On this branch, go to the http://calypso.localhost:3000/home/ or its equivalent on the live link.
• See the updated Reader icon. Visually inspect the spacing and ensure it looks correct.
• Click on the Reader icon to see the active state.
• Test the mobile phone view as well, in which the label should be hidden and the icon should show up.

Before

After

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)?

[github-actions]

Calypso Live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-130058

Jetpack Cloud live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-130058&env=jetpack

Automattic for Agencies live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-130058&env=a8c-for-agencies

[matticbot]

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 update/reader-icon on your sandbox.

[matticbot]

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

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

name         parsed_size           gzip_size
entry-main        +107 B  (+0.0%)      +18 B  (+0.0%)
entry-login       +107 B  (+0.0%)      +18 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.

[davemart-in]

@spsiddarthan on mobile, I'm seeing the label:

It should be hidden here.

I think you can resolve this be adding the masterbar__item-content class, like this:

<span class="masterbar__icon-label masterbar__item-content">Reader</span>

[spsiddarthan]

Thanks, @davemart-in, I am on it.

[davemart-in]

LGTM. Feel free to merge it whenever. Thanks!

[holdercp]

I'm requesting changes because I think the <Item/> component can take the label as its child. Something like

<Item
...
icon={...}
>
  <span class="masterbar__icon-label masterbar__item-content">
	{ translate( 'Reader' ) }
  </span>
</Item>

I might be missing something but it looks like that will be a better place for the label and the icon prop can be used for just the icon.

The desired result looks great but I think this should help keep the code a little tidier.

[spsiddarthan]

I'm requesting changes because I think the component can take the label as its child. Something like

Thank you, @holdercp, that's much cleaner, indeed. I have made the changes and tested it.

I am happy to merge this when I start my day tomorrow, but feel to free to merge it if it's working well for you as well.

[holdercp]

Looks great!

[ciampo]

@holdercp 👋 Flagging the incorrect use of the class prop — in React, the className property should be used instead. This is also causing an error in the browser console: Warning: Invalid DOM property 'class'., which looks like it got missed during the PR review.

I'm going to address this as part of #98510, I'm going to add the folks who worked on this PR as reviewers. Thank you in advance!

[holdercp]

@ciampo Ah, totally slipped by me. Thank you!

[spsiddarthan]

Thank you for catching that @ciampo - I totally missed it, I see the PR is in draft, I am happy to make a different PR as well to fix the warning in a few hours

[spsiddarthan]

I thought I will make a quick PR to get rid of the warning, @ciampo, I see you're improving a few other things as well in your PR, happy to test those out if they're still necessary.