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

Reader sidebar - allow default selections for expanding submenu stream groups. #99477

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

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Feb 7, 2025

Related to # #97993

Proposed Changes

!!?? - in testing/review - Consider if the changes and conditions for applying selection and toggling the menu open or closed make sense. Are there any behavioral cases we can adjust that would make this better?

Adds the ability for our expandable menu items to direct to a defaultSelection when being opened and not being marked as already selected. This should not have any behavioral changes for implementations not using the new props.

  • Adds defaultSelection and isSelected props to the expandable menu component.
  • Adds default selection functionality to the expandable menu component: When the menu is expanded, we route to the defaultSelection IF all 3 conditions are met: defaultSelection exists, the menu is not already open, a child of the menu is not already selected (handled by the isSelected prop).
  • Adds interaction to the expandable icon itself: this triggers the default onClick functionality (toggling the expandable menu without any selection functionality).
  • Adds defaultSelection and isSelected support from the various expandable areas in the reader - Recent, lists, tags, organizations.

BEFORE
reader-selection-before

AFTER
reader-selection-after

Why are these changes being made?

  • Reduce clicks required for reader users navigating between streams.

Testing Instructions

Go to the reader:

  • Test the various expandable menu groups in the reader sidebar: Recent, Lists, Tags, Orgs (A8C, P2, etc.).
  • Verify that:
    • Clicking Region 1 (Red) of a collapsed menu who does not have a child selected loads the first option in that menu and expands the menu.
      • For 0 lists this should load the create list page, for 0 tags this should load the /tags page.
    • Clicking Region 1 (Red) of a collapsed menu who does have a child selected does not change the selection in the menu but does expand the menu.
    • Clicking Region 1 (Red) of an expanded submenu group does not change selection in any case, but does collapse the menu.
    • Clicking Region 2 (Purple) will toggle the expanded menu opened/closed without any selection change in any case.
    • Verify these all work as expected through keyboard flows.
  • Put your design hat or user empathy hat on, consider whether if this is an improvement and if the interactions make sense. Or if we should rethink the rules of conditional selection.

Go to the site sidebar:

  • Go to my home for a site in calypso.
  • Verify the sidebar menu items for menu groups (Appearance, users, etc.) continue to work as they do today and have no changes.
  • Smoke test between default/classic calypso views.

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

@matticbot
Copy link
Contributor

matticbot commented Feb 7, 2025

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

Sections (~70 bytes added 📈 [gzipped])

name                               parsed_size           gzip_size
woocommerce-installation                +296 B  (+0.1%)      +70 B  (+0.1%)
woocommerce                             +296 B  (+0.1%)      +70 B  (+0.1%)
themes                                  +296 B  (+0.0%)      +70 B  (+0.0%)
theme                                   +296 B  (+0.0%)      +70 B  (+0.0%)
subscribers                             +296 B  (+0.0%)      +70 B  (+0.0%)
stats                                   +296 B  (+0.0%)      +70 B  (+0.0%)
staging-site                            +296 B  (+0.0%)      +70 B  (+0.0%)
sites-dashboard                         +296 B  (+0.0%)      +70 B  (+0.0%)
site-tools                              +296 B  (+0.0%)      +70 B  (+0.0%)
site-settings                           +296 B  (+0.0%)      +70 B  (+0.0%)
site-purchases                          +296 B  (+0.0%)      +70 B  (+0.0%)
site-performance                        +296 B  (+0.0%)      +70 B  (+0.0%)
site-overview                           +296 B  (+0.0%)      +70 B  (+0.0%)
site-monitoring                         +296 B  (+0.0%)      +70 B  (+0.0%)
site-marketing                          +296 B  (+0.0%)      +70 B  (+0.0%)
site-logs                               +296 B  (+0.0%)      +70 B  (+0.0%)
settings-writing                        +296 B  (+0.1%)      +70 B  (+0.0%)
settings-security                       +296 B  (+0.1%)      +70 B  (+0.0%)
settings-reading                        +296 B  (+0.1%)      +70 B  (+0.1%)
settings-podcast                        +296 B  (+0.1%)      +70 B  (+0.0%)
settings-performance                    +296 B  (+0.1%)      +70 B  (+0.0%)
settings-newsletter                     +296 B  (+0.1%)      +70 B  (+0.0%)
settings-jetpack                        +296 B  (+0.1%)      +70 B  (+0.0%)
settings-discussion                     +296 B  (+0.1%)      +70 B  (+0.1%)
settings                                +296 B  (+0.0%)      +70 B  (+0.0%)
scan                                    +296 B  (+0.0%)      +70 B  (+0.0%)
purchases                               +296 B  (+0.0%)      +70 B  (+0.0%)
promote-post-i2                         +296 B  (+0.0%)      +70 B  (+0.0%)
preview                                 +296 B  (+0.1%)      +70 B  (+0.1%)
posts-custom                            +296 B  (+0.0%)      +70 B  (+0.0%)
posts                                   +296 B  (+0.0%)      +70 B  (+0.0%)
plugins                                 +296 B  (+0.0%)      +70 B  (+0.0%)
plans                                   +296 B  (+0.0%)      +70 B  (+0.0%)
people                                  +296 B  (+0.0%)      +70 B  (+0.0%)
pages                                   +296 B  (+0.1%)      +70 B  (+0.0%)
migrate                                 +296 B  (+0.1%)      +70 B  (+0.1%)
media                                   +296 B  (+0.0%)      +70 B  (+0.0%)
marketplace                             +296 B  (+0.1%)      +70 B  (+0.0%)
marketing                               +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-social                          +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-search                          +296 B  (+0.1%)      +70 B  (+0.0%)
jetpack-connect                         +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-cloud-settings                  +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-cloud-pricing                   +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-cloud-plugin-management         +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-cloud-overview                  +296 B  (+0.1%)      +70 B  (+0.1%)
jetpack-cloud-features-comparison       +296 B  (+0.0%)      +70 B  (+0.0%)
jetpack-cloud                           +296 B  (+0.1%)      +70 B  (+0.1%)
import                                  +296 B  (+0.0%)      +70 B  (+0.0%)
hosting-features                        +296 B  (+0.0%)      +70 B  (+0.0%)
hosting                                 +296 B  (+0.0%)      +70 B  (+0.0%)
home                                    +296 B  (+0.0%)      +70 B  (+0.0%)
gutenberg-editor                        +296 B  (+0.1%)      +70 B  (+0.0%)
google-my-business                      +296 B  (+0.1%)      +70 B  (+0.0%)
github-deployments                      +296 B  (+0.0%)      +70 B  (+0.0%)
export                                  +296 B  (+0.1%)      +70 B  (+0.1%)
email                                   +296 B  (+0.0%)      +70 B  (+0.0%)
earn                                    +296 B  (+0.0%)      +70 B  (+0.0%)
domains                                 +296 B  (+0.0%)      +70 B  (+0.0%)
customize                               +296 B  (+0.1%)      +70 B  (+0.1%)
concierge                               +296 B  (+0.1%)      +70 B  (+0.0%)
comments                                +296 B  (+0.0%)      +70 B  (+0.0%)
checkout                                +296 B  (+0.0%)      +70 B  (+0.0%)
backup                                  +296 B  (+0.0%)      +70 B  (+0.0%)
add-ons                                 +296 B  (+0.1%)      +70 B  (+0.1%)
activity                                +296 B  (+0.0%)      +70 B  (+0.0%)
a8c-for-agencies-plugins                +296 B  (+0.0%)      +70 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 (~260 bytes added 📈 [gzipped])

name                               parsed_size           gzip_size
async-load-calypso-reader-sidebar       +903 B  (+1.0%)     +260 B  (+1.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.

@matticbot
Copy link
Contributor

matticbot commented Feb 7, 2025

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 try/update-expandable-sidebar-menu-to-have-default-selections on your sandbox.

@Addison-Stavlo Addison-Stavlo changed the title sidebar expandable items - try supporting a default selection Reader sidebar - allow default selections for expanding submenu stream groups. Feb 10, 2025
@Addison-Stavlo Addison-Stavlo self-assigned this Feb 10, 2025
@Addison-Stavlo Addison-Stavlo marked this pull request as ready for review February 10, 2025 18:43
@Addison-Stavlo Addison-Stavlo requested a review from a team as a code owner February 10, 2025 18:43
@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 Feb 10, 2025
@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Feb 10, 2025

On the surface it feels good. But the edge cases im not as sure about.

For example, I wonder about this kind of case: say we are on an unrelated stream and go to click “Recent” here where it is expanded.

Currently on the PR it just collapses that recent menu without selecting anything.

Alternatively, I wonder if it should not collapse the menu in that case and apply the default selection instead. Maybe “Recent” should only collapse the menu if one of its children is selected? or not at all? 🤔

On one hand the current behavior makes sense because all the options are visible, it doesn't make too much sense to auto select one. On the other hand, maybe it feels more consistent if its always selecting a child when its children aren’t yet selected.

@davemart-in
Copy link
Contributor

Yeah, thanks for highlighting this.

Let's simplify it. Let's 100% remove the collapse functionality from the left (red side):

image

The only thing that will collapse it is if it's expanded and you click the purple dropdown portion.

I think it will, but does that resolves your edge case?

@Addison-Stavlo
Copy link
Contributor Author

I think it will, but does that resolves your edge case?

I guess thats a matter of opinion. The edge cases will be there regardless, I think we need to determine the best approach and most acceptable trade offs in general for it.

Let's 100% remove the collapse functionality from the left (red side): The only thing that will collapse it is if it's expanded and you click the purple dropdown portion.

The downside here is we then have a button that stops doing anything under certain conditions. If im understanding that case, click "Recent" would:

  • If its not expanded, it will expand the area.
  • If not selected, it will select the default.
  • If its expanded and selected, the button becomes inert. (no collapse functionality)

Is that button being inert at times a UX concern? (Im honestly not sure, maybe its fine). Do we see that as better than the current state where clicking "Recent" would:

  • Toggle collapsed/expanded either way.
  • Select the default when expanding, if the group is not yet selected.

@davemart-in
Copy link
Contributor

Yeah...

You're right.

I'm tempted to say let's just close this out.

@robertbpugh
Copy link

It seems like having the key functionality be reachable in one click would be more important than the edge cases you mentioned. I will see if I can test it.

@robertbpugh
Copy link

Thanks for the testing help. My thoughts:

  • Having Recent list posts in one click seems like a win to me. I imagine this is the most or second-most clicked item in the Reader sidebar and we're cutting clicks in half to accomplish what you likely wanted to do.
  • I agree it's bad to have the expander Recent section, click on Recent, and have it collapse. I like Dave's proposed solution to have a click on the word Recent do nothing, and the arrow collapses the section. Feels intuitive to me, and I'd prefer that edge case vs. continuing with 2 clicks to view Recent.

@davemart-in
Copy link
Contributor

This latest change feels nice @Addison-Stavlo.

One thing I noticed was the selected state is different between Recent and the other dropdowns in the left nav. Recent uses bold and the others don't. Thought on making it bold consistently for all of them?

@Addison-Stavlo
Copy link
Contributor Author

This latest change feels nice @Addison-Stavlo.

Im confused how you tested them because I think the calypso build is still building. 😅

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Feb 11, 2025

One thing I noticed was the selected state is different between Recent and the other dropdowns in the left nav. Recent uses bold and the others don't. Thought on making it bold consistently for all of them?

I haven't touched those styles at all here. Im partial to the blue myself, but maybe better to handle separately. Would we change the individual items selected to bold instead of blue as well? (and then would we need bold icon weights?). Maybe a separate issue for those changes would be good.

Screenshot 2025-02-11 at 2 57 46 PM

(Edit: selected items in the global sidebar on dotcom dashboard use this blue as well, I would say we should be consistent and use the blue instead of bold if anything).

@Addison-Stavlo
Copy link
Contributor Author

The calypso live branch is still being generated, but I did push a commit to prototype the proposed functionality. I still might need to refactor a little, and need to hide the cursor pointer style on the "red" area when it is inert.

But curious for thoughts on the behavior now, this should currently behave as:

  • Clicking "Recent" (or that area on any of the collapsable) should:
    • If its not expanded, it will expand the area.
    • If not selected, it will select the default.
    • If its expanded and selected, the button becomes inert. (no collapse functionality)
  • Clicking the arrow on the right should always toggle without selection.

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Feb 11, 2025

The live branch should be up now.

Im wondering if clicking "Recent" when the menu is already expanded and has one of its items currently selected should either:
A) Do nothing (intert button), current behavior and why we would still need to remove cursor style in this condition.
OR
B) Re-select the default selection for that group. (So in case of "Recent", clicking "Recent" when one of the sites in Recent is selected would change the selection back to "All")
🤔

B might be a little more consistent maybe?

@davemart-in
Copy link
Contributor

Im confused how you tested them because I think the calypso build is still building. 😅

Yeah, i didn't have the latest... :-)

I haven't touched those styles at all here. Im partial to the blue myself

Sorry, I think we're talking about two different things. I'm talking about the sub menu active links:

CleanShot 2025-02-11 at 15 49 01@2x

vs.

CleanShot 2025-02-11 at 15 49 25@2x

It feels weird that they're styled differently. I like the bold one better. The blue link doesn't look selected to me.

Clicking "Recent" (or that area on any of the collapsable) should:
If its not expanded, it will expand the area.
If not selected, it will select the default.
If its expanded and selected, the button becomes inert. (no collapse functionality)
Clicking the arrow on the right should always toggle without selection.

This seems to work. To Rob's point, it remedies the double click issue.

Im wondering if clicking "Recent" when the menu is already expanded and has one of its items currently selected should either:
A) Do nothing (intert button), current behavior and why we would still need to remove cursor style in this condition.
OR
B) Re-select the default selection for that group. (So in case of "Recent", clicking "Recent" when one of the sites in Recent is selected would change the selection back to "All")

Yeah, I like B.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants