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: upgraded design screenshots #43561

Merged
merged 1 commit into from
Jul 3, 2020

Conversation

simison
Copy link
Member

@simison simison commented Jun 23, 2020

Changes proposed in this Pull Request

Re-ran the script to generate screenshots (while testing #43559).

To review, I recommend some tool such as Github desktop that shows image differences side-by-side. Sadly Github itself doesn't. :-(

Ah screw that, I'm just gonna screenshot all these:

Preview links, screenshots and notes for each design

See screenshots (these are outdated, see latest comments for fresh ones

Vesta
Screenshot 2020-06-23 at 14 14 28
Bolded title (this is in many designs so I'm not gonna mention it again)

Balasana
Screenshot 2020-06-23 at 12 44 59
Different spacing

Barnsbury
Screenshot 2020-06-23 at 12 44 21
Underlined title, side-spacing gone.

Bowen
Screenshot 2020-06-23 at 12 43 56
Underline

Coutoire
Screenshot 2020-06-23 at 12 43 02

Doyle
Screenshot 2020-06-23 at 12 42 43
Side margin issue.

Easley
Screenshot 2020-06-23 at 12 42 19
Side margin issue.

Edison
Screenshot 2020-06-23 at 12 41 54

Rivington
Screenshot 2020-06-23 at 12 40 09

Rockfield
Screenshot 2020-06-23 at 12 39 18
💥 prolly needs similar treatment to D45189-code

Stratford
Screenshot 2020-06-23 at 14 18 40
Image showed up.

Cassel
image

Camdem
image

Reynolds
image

Overton
image

Brice
image

Alves
image

Mayland
image

Maywood
image

Leven
image

Gibbs
image

The two FSE demos we can probably ignore for now:

Testing instructions

See designs step at /new

@ianstewart could you look these through and see if some of the changes are problems or is everything expected? I didn't have time to look into previews nor real sites yet if the problems are present in those too.

@simison simison added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] New Onboarding previously called Gutenboarding labels Jun 23, 2020
@simison simison requested a review from a team June 23, 2020 09:46
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@simison simison marked this pull request as ready for review June 23, 2020 11:27
@simison simison requested a review from ianstewart June 23, 2020 11:29
@ianstewart
Copy link
Contributor

could you look these through and see if some of the changes are problems or is everything expected?

Only the side-margin issues with Doyle and Easly and the giant hand of Rockfield are unexpected.

@ramonjd
Copy link
Member

ramonjd commented Jun 24, 2020

Only the side-margin issues with Doyle and Easly and the giant hand of Rockfield are unexpected.

I had to generate this one manually for #42354. I could not escape the giant hand. :(

I used this Chrome extension

@simison
Copy link
Member Author

simison commented Jun 25, 2020

@ramonjd do you know what's going on with missing side margins in some templates? Is it a specific theme broken somehow? Cc @kjellr

@andrewserong
Copy link
Member

@simison could the side margins be related to the recent Layout Grid version bump? (I haven't checked the changelog, but it just occurred to me because of the issue we had with enqueuing the CSS for the template/demo endpoint)

@simison
Copy link
Member Author

simison commented Jun 25, 2020

could the side margins be related to the recent Layout Grid version bump?

@Automattic/tinker 👋 know about any changes that might've affected? We had this suspicion once before but found it to be issue in Gutenberg core instead.

cc @youknowriad any editor margin changes in core that might've broken things?

Edit: I tested that this happens also with Group block in full-width mode, so looks like it's a core issue.

Example breakage:

Not sure why it's only these two designs.

@jasmussen
Copy link
Member

@simison
Copy link
Member Author

simison commented Jun 25, 2020

@jasmussen .com runs Gutenberg 8.3 though — if I understand correct, those would affect only 8.4?

@jasmussen
Copy link
Member

Indeed :|

Is that the Layout Grid block used in those screenshots, and does it appear related to that? We have some end-gutter paddings for that block which may have a bug, if it's layout-grid only.

@johngodley
Copy link
Member

We do have a report of the outmost gutter being missing in the Layout Grid, but in the editor only, not the front end: Automattic/block-experiments#103

Edit: I tested that this happens also with Group block in full-width mode, so looks like it's a core issue.

I'm not sure of the timeline of this edit. Do we suspect the grid block still?

@johngodley
Copy link
Member

In terms of changes to the grid block, the recent update was fairly minor and focussed on the editor only.

  • Fix block inserter to show inside a grid column
  • Fix vertical margin in editor so it better matches the display
  • Fix CSS loading so it is only added when block is used
  • Fix grid lines appearing when inner block is selected

@sirreal
Copy link
Member

sirreal commented Jun 25, 2020

I recall similar issues recently and dug up these diffs that may be related:

  • D42350-code
  • D42335-code

@razvanpapadopol
Copy link

@johngodley I can confirm the side margin issue being discussed here is related to Automattic/block-experiments#103

After some investigation I see the following styles are not loaded inside the editor but are loaded on the frontend, as opposed to the comment at the top of the file:
https://github.com/Automattic/block-experiments/blob/master/blocks/layout-grid/style.scss#L16-L17

@simison
Copy link
Member Author

simison commented Jun 26, 2020

@johngodley @jasmussen could you look into style-loading issue in the layout-grid block? Seems like something is going on there.

A few days ago we had to hot-fix this for the previews too (D45267-code) and in initial investigation it seemed like it would be previews-issue, but perhaps it's more wider problem?

Thanks for investigation @razvanpapadopol !

@johngodley
Copy link
Member

@simison sure, it's being tracked in Automattic/block-experiments#103. It doesn't affect the front end.

@johngodley
Copy link
Member

Noting that the style difference in the editor for the layout grid is now resolved.

@simison simison force-pushed the gutenboarding/update-screenshots branch from 42a4dad to 6a39379 Compare July 2, 2020 11:58
@simison simison force-pushed the gutenboarding/update-screenshots branch from 6a39379 to 3355792 Compare July 2, 2020 13:21
@simison
Copy link
Member Author

simison commented Jul 2, 2020

I regenerated screenshots so this now has latest, and I'm just excluding the "large hand" rockfield_rockfield_rockfield.jpg.

See screenshots Screenshot 2020-07-02 at 15 01 35 Screenshot 2020-07-02 at 15 01 28 Screenshot 2020-07-02 at 15 01 17 Screenshot 2020-07-02 at 15 00 53 Screenshot 2020-07-02 at 15 00 50 Screenshot 2020-07-02 at 15 00 47 Screenshot 2020-07-02 at 15 00 42 Screenshot 2020-07-02 at 15 00 39 Screenshot 2020-07-02 at 15 00 30 Screenshot 2020-07-02 at 15 00 26 Screenshot 2020-07-02 at 15 00 22 Screenshot 2020-07-02 at 15 00 18 Screenshot 2020-07-02 at 15 00 12 Screenshot 2020-07-02 at 15 00 07 Screenshot 2020-07-02 at 14 59 59 Screenshot 2020-07-02 at 14 59 54 Screenshot 2020-07-02 at 14 59 48 Screenshot 2020-07-02 at 14 59 44 Screenshot 2020-07-02 at 14 59 40 Screenshot 2020-07-02 at 14 59 35 Screenshot 2020-07-02 at 14 59 31 Screenshot 2020-07-02 at 14 59 26

@ramonjd
Copy link
Member

ramonjd commented Jul 2, 2020

and I'm just excluding the "large hand"

THE HAND OF DOOM!

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Yoda says TMLG!

@ramonjd ramonjd merged commit c1da5df into master Jul 3, 2020
@ramonjd ramonjd deleted the gutenboarding/update-screenshots branch July 3, 2020 03:09
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] New Onboarding previously called Gutenboarding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants