PR: Improve dividers between cards on mobile #90229

davemart-in · 2024-05-02T19:53:16Z

Description

This relates to https://github.com/Automattic/loop/issues/177

Dividers between cards could probably benefit from being 100% width and slightly darker, again to just help you see at a glance as you're flipping through your feed when you've moved from one card to the next:

Before

After

matticbot · 2024-05-02T19:54:54Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-05-02T20:02:02Z

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

Sections (~186 bytes added 📈 [gzipped])

name                             parsed_size           gzip_size
woocommerce-installation               +16 B  (+0.0%)      +10 B  (+0.0%)
themes                                 +16 B  (+0.0%)      +11 B  (+0.0%)
subscribers                            +16 B  (+0.0%)       +9 B  (+0.0%)
stats                                  +16 B  (+0.0%)      +12 B  (+0.0%)
site-purchases                         +16 B  (+0.0%)       +7 B  (+0.0%)
site-blocks                            +16 B  (+0.0%)      +12 B  (+0.0%)
settings-writing                       +16 B  (+0.0%)       +5 B  (+0.0%)
settings-security                      +16 B  (+0.0%)      +14 B  (+0.0%)
settings-reading                       +16 B  (+0.0%)       +4 B  (+0.0%)
settings-podcast                       +16 B  (+0.0%)       +5 B  (+0.0%)
settings-performance                   +16 B  (+0.0%)       +4 B  (+0.0%)
settings-newsletter                    +16 B  (+0.0%)       +5 B  (+0.0%)
settings-jetpack                       +16 B  (+0.0%)       +5 B  (+0.0%)
settings-discussion                    +16 B  (+0.0%)       +4 B  (+0.0%)
settings                               +16 B  (+0.0%)       +8 B  (+0.0%)
security                               +16 B  (+0.0%)      +12 B  (+0.0%)
scan                                   +16 B  (+0.0%)       +7 B  (+0.0%)
reader                                 +16 B  (+0.0%)       +7 B  (+0.0%)
purchases                              +16 B  (+0.0%)      +12 B  (+0.0%)
privacy                                +16 B  (+0.0%)      +12 B  (+0.0%)
posts-custom                           +16 B  (+0.0%)       +5 B  (+0.0%)
posts                                  +16 B  (+0.0%)       +5 B  (+0.0%)
plugins                                +16 B  (+0.0%)      +11 B  (+0.0%)
plans                                  +16 B  (+0.0%)      +11 B  (+0.0%)
people                                 +16 B  (+0.0%)       +8 B  (+0.0%)
pages                                  +16 B  (+0.0%)       +5 B  (+0.0%)
notification-settings                  +16 B  (+0.0%)      +12 B  (+0.0%)
media                                  +16 B  (+0.0%)       +5 B  (+0.0%)
me                                     +16 B  (+0.0%)      +12 B  (+0.0%)
marketing                              +16 B  (+0.0%)       +4 B  (+0.0%)
jetpack-cloud-settings                 +16 B  (+0.0%)       +5 B  (+0.0%)
jetpack-cloud-plugin-management        +16 B  (+0.0%)      +11 B  (+0.0%)
jetpack-cloud-agency-sites-v2          +16 B  (+0.0%)       +6 B  (+0.0%)
import                                 +16 B  (+0.0%)       +8 B  (+0.0%)
hosting                                +16 B  (+0.0%)      +11 B  (+0.0%)
home                                   +16 B  (+0.0%)       +7 B  (+0.0%)
help                                   +16 B  (+0.0%)      +12 B  (+0.0%)
google-my-business                     +16 B  (+0.0%)       +5 B  (+0.0%)
github-deployments                     +16 B  (+0.0%)       +9 B  (+0.0%)
export                                 +16 B  (+0.0%)       +7 B  (+0.0%)
email                                  +16 B  (+0.0%)       +6 B  (+0.0%)
earn                                   +16 B  (+0.0%)      +12 B  (+0.0%)
domains                                +16 B  (+0.0%)       +6 B  (+0.0%)
developer                              +16 B  (+0.0%)      +12 B  (+0.0%)
comments                               +16 B  (+0.0%)       +6 B  (+0.0%)
backup                                 +16 B  (+0.0%)       +6 B  (+0.0%)
add-ons                                +16 B  (+0.0%)       +7 B  (+0.0%)
activity                               +16 B  (+0.0%)       +6 B  (+0.0%)
account-close                          +16 B  (+0.0%)      +12 B  (+0.0%)
account                                +16 B  (+0.0%)      +12 B  (+0.0%)
a8c-for-agencies-sites                 +16 B  (+0.0%)       +6 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 (~16 bytes added 📈 [gzipped])

name                              parsed_size           gzip_size
async-load-store-app-store-stats        +16 B  (+0.0%)       +6 B  (+0.0%)
async-load-design-playground            +16 B  (+0.0%)      +10 B  (+0.0%)
async-load-design                       +16 B  (+0.0%)      +10 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.

matticbot · 2024-05-03T07:18:12Z

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

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

blaze-dashboard
editing-toolkit
odyssey-stats
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/reader-mobile-dividers on your sandbox.

Addison-Stavlo · 2024-05-03T12:23:41Z

At first glance im confused why we are changing 11 separate files to achieve this?

Addison-Stavlo · 2024-05-03T12:31:04Z

I ran into something weird I cant seem to repro elsewhere. If i go to conversations, collapse the browser width to mobile, and slowly widen it back out... the sidebar gets stuck in collapsed mode?

I wouldn't expect this looking at the changes but cant seem to repro on production. Are you seeing this as well?

…-calypso into fix/reader-mobile-dividers

davemart-in · 2024-05-03T12:44:57Z

I wouldn't expect this looking at the changes but cant seem to repro on production. Are you seeing this as well?

I am. How weird is that?

Digging in...

davemart-in · 2024-05-03T12:47:50Z

@Addison-Stavlo though I'm not seeing this locally.

davemart-in · 2024-05-03T12:49:55Z

Also noticed that it catches up and resolves itself after like 5 seconds. That's weird. Looks like the .is-global-sidebar-collapsed class removal is being delayed for some reason?

Addison-Stavlo · 2024-05-03T13:17:46Z

Also noticed that it catches up and resolves itself after like 5 seconds. That's weird. Looks like the .is-global-sidebar-collapsed class removal is being delayed for some reason?

If we look at the selector causing this in #90280 there are 2 issues. One is that it can return true outside of the sites dashboard sections (it can be true simply for its breakpoint and redesign flag). That PR is updating the check to ensure we are on the sites dashboard.

The second issue is the use of the isWithinBreakpoint function. Because this is simply the isWithinBreakpoint function and not a useBreakpoint hook within the component, the changing breakpoint will not cause the component to rerender and update. Instead, it doesn't recalculate until the component rerenders for a separate reason. That would be the delay you see I think.

We could resolve the second issue delay by updating the components that use this selector. Instead of relying on isWithinBreakpoint within the selector, we use useBreakpoint in the components that are running that selector instead. Although im not sure if the expanding/contracting viewport size immediate responsiveness is a common user flow to worry about, and once things are limited to the sites dashboard where this collapsed version works as expected the delay may not be a big deal. 🤔

davemart-in · 2024-05-03T13:21:55Z

Yeah, I suspect your PR resolves this enough for now.

Addison-Stavlo · 2024-05-03T13:36:32Z

Ok, back to the changes here 😅.

In general the changes look great! There is one thing im noticing in the header for the search stream though.

BEFORE

AFTER

Notice both the position change of:

The relevance/date tabs above the input
The "sites" tab header below the input

client/reader/search-stream/index.jsx

Fixed daily prompts

davemart-in · 2024-05-03T15:25:31Z

Okay @Addison-Stavlo I think this is ready for another review.

Addison-Stavlo · 2024-05-03T17:06:42Z

Looking good. There is one other place I see a regression. If we look at the subscriptions tab on the following feed in mobile.

BEFORE

AFTER

davemart-in · 2024-05-03T20:34:09Z

Okay I think this one is ready for another review. Thanks!

Addison-Stavlo

Looks good.

Unit tests are failing because of a typescript check (it isnt expecting your new 'style' object on props). Yay typescript. I pushed a change so hopefully that should be good.

Addison-Stavlo

✅ when tests pass

First pass

749a2ad

davemart-in self-assigned this May 2, 2024

davemart-in requested a review from a team as a code owner May 2, 2024 19:53

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 2, 2024

davemart-in requested a review from a team May 2, 2024 20:49

irst pass

c4a05f5

roo2 force-pushed the fix/reader-mobile-dividers branch from 749a2ad to c4a05f5 Compare May 3, 2024 07:11

Merge branch 'fix/reader-mobile-dividers' of github.com:Automattic/wp…

0696867

…-calypso into fix/reader-mobile-dividers

Addison-Stavlo mentioned this pull request May 3, 2024

Global Sidebar - prevent collapsed state from bleeding into other areas. #90280

Merged

7 tasks

Addison-Stavlo reviewed May 3, 2024

View reviewed changes

client/reader/search-stream/index.jsx Show resolved Hide resolved

davemart-in added 4 commits May 3, 2024 11:17

Fixed issue with search header

dfe963d

Fixed section headers

5df0ba6

Update style.scss

7ffd179

Fixed daily prompts

Fixed sites tab headers

4523aab

Refactored a number of things

045e775

add style to prop interface type

7107bb3

Addison-Stavlo reviewed May 3, 2024

View reviewed changes

Addison-Stavlo approved these changes May 3, 2024

View reviewed changes

davemart-in added 6 commits May 4, 2024 08:58

Likes section header tweak

5964b7b

Conversations header section tweak

d7eeb85

Fixed back buttons

d6d7923

Fixed comments padding

f9c4a0f

Fixed tag stream header lack of padding

7babaf8

Fixed A8c section header padding

8b27f5c

davemart-in merged commit 1d7b295 into trunk May 4, 2024
11 checks passed

davemart-in deleted the fix/reader-mobile-dividers branch May 4, 2024 13:27

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 May 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Improve dividers between cards on mobile #90229

PR: Improve dividers between cards on mobile #90229

davemart-in commented May 2, 2024

matticbot commented May 2, 2024 •

edited

Loading

matticbot commented May 2, 2024 •

edited

Loading

matticbot commented May 3, 2024 •

edited

Loading

Addison-Stavlo commented May 3, 2024

Addison-Stavlo commented May 3, 2024 •

edited

Loading

davemart-in commented May 3, 2024

davemart-in commented May 3, 2024

davemart-in commented May 3, 2024

Addison-Stavlo commented May 3, 2024

davemart-in commented May 3, 2024

Addison-Stavlo commented May 3, 2024 •

edited

Loading

davemart-in commented May 3, 2024

Addison-Stavlo commented May 3, 2024 •

edited

Loading

davemart-in commented May 3, 2024

Addison-Stavlo left a comment

Addison-Stavlo left a comment

PR: Improve dividers between cards on mobile #90229

PR: Improve dividers between cards on mobile #90229

Conversation

davemart-in commented May 2, 2024

Description

Description

Before

After

matticbot commented May 2, 2024 • edited Loading

matticbot commented May 2, 2024 • edited Loading

matticbot commented May 3, 2024 • edited Loading

Addison-Stavlo commented May 3, 2024

Addison-Stavlo commented May 3, 2024 • edited Loading

davemart-in commented May 3, 2024

davemart-in commented May 3, 2024

davemart-in commented May 3, 2024

Addison-Stavlo commented May 3, 2024

davemart-in commented May 3, 2024

Addison-Stavlo commented May 3, 2024 • edited Loading

davemart-in commented May 3, 2024

Addison-Stavlo commented May 3, 2024 • edited Loading

davemart-in commented May 3, 2024

Addison-Stavlo left a comment

Choose a reason for hiding this comment

Addison-Stavlo left a comment

Choose a reason for hiding this comment

matticbot commented May 2, 2024 •

edited

Loading

matticbot commented May 2, 2024 •

edited

Loading

matticbot commented May 3, 2024 •

edited

Loading

Addison-Stavlo commented May 3, 2024 •

edited

Loading

Addison-Stavlo commented May 3, 2024 •

edited

Loading

Addison-Stavlo commented May 3, 2024 •

edited

Loading