-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update dataviews package version to latest #93503
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~2217 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~33718 bytes added 📈 [gzipped])
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 (~10652 bytes removed 📉 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
client/a8c-for-agencies/components/items-dashboard/items-dataviews/index.tsx
Show resolved
Hide resolved
@@ -101,7 +103,7 @@ const ItemsDataViews = ( { data, isLoading = false, className }: ItemsDataViewsP | |||
} | |||
onSelectionChange={ data.onSelectionChange } | |||
onChangeView={ data.setDataViewsState } | |||
supportedLayouts={ [ 'table' ] } | |||
defaultLayouts={ { table: {} } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mayne we should support the array notation upstream as a shortcut.
|
||
.dataviews-wrapper { | ||
// Maybe move upstream to the gutenberg repository | ||
width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, it seems the dataviews take 0 width. Maybe this style should be moved directly to the dataviews package in the Gutenberg repository.
6f4c156
to
4a665f6
Compare
client/a8c-for-agencies/sections/sites/sites-dashboard-provider.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency and predictability, I'd recommend updating @wordpress/dataviews
to the version that corresponds to the current monorepo package versions, and then update all packages together in #92571. Let me know if you need any help with that.
I agree that that would be better in terms of process but in this case, I think it's fine to make an exception because otherwise we'll do two big migrations and I'd rather do it once into a fully typed package. (And most of the work is already done) |
@youknowriad I'm struggling to find a futureproof way to hide the Plan column responsively on mobile screen widths (<960px). I tried changing the column width / maxWidth / minWidth to 0px in the view layout, but that didn't hide it. It seems like overkill to hide and show it as the window width changes. Is there a DataViews way to do this? |
@allilevine Maybe the best way is to update the |
paddingTop: '2px', | ||
color: '#3858e9 !important', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the DataViews component might be too opinionated with colors, maybe there's a fix to do in Gutenberg here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great 🙏
@allilevine I see that you styled back the search input to how it was in trunk. I agree that it's a better design personally but IMO we should try to be consistent. I don't see why .com's search input should be different from .org's one. Why don't we update the design in .org directly instead? cc @jameskoster as there might be reasons for the current design in .org. |
I agree. Iirc the current design was optimised for the Inserter, which was one of the only |
Sounds good! I prioritized that regression because it's inconsistent with Domains and Plugins. But Themes is also currently inconsistent, and if our first priority is consistency with core, it makes sense to make the update there. cc'ing @davemart-in @lucasmendes-design on this decision. |
I've noticed the "Add filter" button styles don't communicate well the disabled state:
I've pinpointed the issue to this dotcom override of the core wp-components. To be fair, it's not the "same color", as an tertiary active button gets the
|
5dee987
to
9362069
Compare
Pushed a rebase because there were conflicts with |
While I don't consider these blockers for this PR (because we hack around them), Here's the list of follow-ups that we should be proposing for the Core package:
One follow-up that I think should be done in calypso instead is the untangling of the "site preview" and the "site title" fields and using Other than that, this PR is in a decent state, we need a bit more testing in order to land it. |
I've started working on some of the Core follow-ups while this PR is waiting for review/merge. Here's the first one: Add support for components to the field labels. WordPress/gutenberg#64642 |
Some feedback:
|
By and large, everything looks good! I'm going to call out a few observations. Maybe these are things that need to get fixed upstream. I'm not sure of the severity of these, so I'll just leave the comments and let ya'll sort through them.
|
🟢 Dotcom Sites: it's shippable, in my view.
|
🟢 A4A Sites Modal, A4A Referrals (Overview, Details, For Clients), A4A Team (not in production). |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
This is a long-standing issue I'd love to close: WordPress/gutenberg#57669 |
This reverts commit cfb9373.
Really cool to see this merged. Thanks all for your help. |
Fixes https://github.com/Automattic/jetpack-manage/issues/337
Proposed Changes
Why are these changes being made?
DataViews
in Core. See pe7F0s-21J-p2Status
<calypso>/sites
<agencies>/sites
<agencies>/sites
("add new site" modal)<agencies>/referrals
<agencies>/referrals
(with an item selected)<agencies>/clients/subscriptions
<agencies>/team
<jpcloud>/sites
Dotcom Sites
A4A Sites
A4A Sites Modal
A4A Referrals Overview
A4A Referrals Details
A4A Referrals for clients
data-field-id="actions"
that no longer exists in DataViews.A4A Team
Jetpack Cloud
Testing Instructions
Pre-merge Checklist