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

Gutenboarding: Drop FSE reuse/use MediaCard #38456

Merged
merged 6 commits into from
Dec 17, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Dec 16, 2019

Changes proposed in this Pull Request

  • Removes dependence on sibling monorepo app's components (FSE plugin)
  • Pulls in unpublished Card from @wordpress/components (mostly simple copy)
  • Fixes an issue where a position: fixed from Gutenberg styles breaks the block editor's overflow (no scrolling)
  • Uses simple grid layout from Gutenboarding: Update Template Selector Styling #38403 (props @ockham)
  • Implements very simple list layout designs from API + MediaCard component.
  • Adds simple selected design visual feedback for demonstration

Closes #38403 (supersedes)

Demo

demo

Testing instructions

  • Go to /gutenboarding on (calypso.live or dev branch)
  • Pass the first screen (select a vertical and site title) and click "Next"
  • Play with the design picker ✨

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Dec 16, 2019
@sirreal sirreal requested a review from a team as a code owner December 16, 2019 18:40
@sirreal sirreal self-assigned this Dec 16, 2019
@matticbot
Copy link
Contributor

@sirreal sirreal changed the title Copy and use Card from Gutenberg Gutenboarding: Drop FSE reuse/use MediaCard Dec 16, 2019
@@ -105,7 +105,7 @@ export function Gutenboard() {
value={ [ onboardingBlock.current ] }
settings={ { templateLock: 'all' } }
>
<div className="edit-post-layout__content">
<div className="gutenboard__edit-post-layout-content edit-post-layout__content ">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to do a scrub of these some time soon, to make things more edit-post agnostic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think edit-post-layout__content has been removed from Gutenberg as well. (in WordPress/gutenberg#18044). We had to update our styles in #37863

onClick={ () => setSelectedDesign( t.slug ) }
>
<CardMedia>
<img alt={ t.title } src={ t.preview } />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might add a srcset in a follow-up. Maybe I'll turn this stuff into a util function that returns everything needed for a srcset.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also some prior art for constructing photon-style srcsets in Jetpack Tiled Gallery block:

https://github.com/Automattic/jetpack/blob/ed104a24c0a73f6015abcff1b01d0d3b693b52f3/extensions/blocks/tiled-gallery/utils/index.js#L72-L111

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I think we're dealing with a tighter set of constraints here (already-photonized WP.com images), so I was hoping to get away with a rather simple implementation...

@include breakpoint( '>660px' ) {
// stylelint-disable unit-whitelist
grid-template-columns: repeat( 2, 1fr );
// stylelint-enable unit-whitelist
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably change the linter config to allow this unit /cc @simison

&.is-selected {
transform: scale( 1.1 );
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So concise! 😍

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thanks Jon, awesome job! (I should've listened to you earlier, this is so much more straightforward than messing around with TemplateSelectorControl)

LGTM! 🚢

(I'll update #38403 to only take care of higher-res screenshots/srcset, and the added headings.)

@ockham
Copy link
Contributor

ockham commented Dec 17, 2019

(I'll update #38403 to only take care of higher-res screenshots/srcset, and the added headings.)

Nah I'll probably just base it on this PR.

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 17, 2019
@sirreal sirreal merged commit 7a04a2c into master Dec 17, 2019
@ockham ockham deleted the gutenboarding/try-design-card branch December 17, 2019 09:50
@matticbot
Copy link
Contributor

matticbot commented Dec 18, 2019

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

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

name                 parsed_size           gzip_size
entry-gutenboarding     +25300 B  (+1.0%)    +9103 B  (+1.4%)

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.

ockham added a commit that referenced this pull request Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants