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

Replace site component with dataview combined fields #96657

Merged
merged 19 commits into from
Nov 27, 2024
Merged

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Nov 22, 2024

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

Proposed changes:

Reworks the SiteField component to split site icon away from site title. Site icon is then combined with site-title on the table view and made the primary on list view with icon becoming the media field.

This change allows removal of the css overrides to hide the media and primary fields with display none.

Also fixes styling and disables onclick for deleted sites which was previously opening site stats (the fallback site home url) and taking you out of the sites dashboard with a broken browser back button.

Before

Screenshot 2024-11-26 at 16-44-11 Sites — WordPress com

After

Screenshot 2024-11-26 at 16-43-50 Sites — WordPress com

Testing

  • Load /sites
  • Confirm sorting has no new properties shown in the dataviews settings pop over
  • Confirm clicking title opens and switches sites as before
  • Confirm clicking on the icon opens and switches sites as before
  • Confirm clicking deleted site titles and icons does nothing
  • Confirm resizing the window maintains all columns until mobile which drops all but site title and action columns
  • Confirm loading the page on a mobile screen works as expected
  • Confirm switching sites works as expected on mobile

@matticbot
Copy link
Contributor

matticbot commented Nov 22, 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             +215 B  (+0.0%)      +31 B  (+0.0%)
sites-dashboard          +215 B  (+0.0%)      +31 B  (+0.0%)
site-tools               +215 B  (+0.0%)      +31 B  (+0.0%)
site-settings            +215 B  (+0.0%)      +31 B  (+0.0%)
site-performance         +215 B  (+0.0%)      +31 B  (+0.0%)
site-overview            +215 B  (+0.0%)      +31 B  (+0.0%)
site-monitoring          +215 B  (+0.0%)      +31 B  (+0.0%)
site-marketing           +215 B  (+0.0%)      +31 B  (+0.0%)
site-logs                +215 B  (+0.0%)      +31 B  (+0.0%)
hosting-features         +215 B  (+0.0%)      +31 B  (+0.0%)
hosting                  +215 B  (+0.0%)      +31 B  (+0.0%)
github-deployments       +215 B  (+0.0%)      +31 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.

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.

@lsl lsl marked this pull request as ready for review November 26, 2024 03:44
const siteTitle = isMigrationPending ? translate( 'Incoming Migration' ) : site.title;

return (
<Button
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make this a non-button element? Because:

  • The Site Title is already a focusable element, so having two focusable elements for the same action seems unnecessary.
  • Ensuring the focus state is visible on mobile view seems somewhat tricky (it's not visible currently).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me.

@okmttdhr okmttdhr requested review from oandregal and a team November 26, 2024 07:29
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 26, 2024
@matticbot
Copy link
Contributor

matticbot commented Nov 26, 2024

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

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

  • notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/dv-primary on your sandbox.

@okmttdhr
Copy link
Member

I left a comment, but it looks to be working pretty well!

Copy link
Member

@okmttdhr okmttdhr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I noticed elements are vertically aligned in the list view, which differs from production & Core, but I can follow up on it in #96719.
Screenshot 2024-11-27 at 10 07 45

@okmttdhr
Copy link
Member

I'm merging this PR since it streamlines subsequent changes. If you have any feedback later, @oandregal, we can address it in a follow-up!

@okmttdhr okmttdhr merged commit 08ef6ff into trunk Nov 27, 2024
11 checks passed
@okmttdhr okmttdhr deleted the update/dv-primary branch November 27, 2024 02:31
@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 Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants