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

Block-Editor: reorder Jetpack and CoBlocks categories #37057

Merged
merged 4 commits into from
Oct 28, 2019

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Oct 25, 2019

Changes proposed in this Pull Request

Part of #35876 this moves Jetpack and CoBlocks categories below other categories.

Note that the core/blocks store uses the array order of whatever we dispatched via setCategories, so doing things like changing a block's category slug name or translated title currently do not affect ordering.

Before After
63965105-504c9700-ca4d-11e9-89a7-e9754c3d4db7 Screen Shot 2019-10-24 at 6 12 00 PM

Screen Shot 2019-10-24 at 6 11 52 PM

Questions / Notes

  • Open question for @apeatling and @simison, would we like this to affect all site types or just Atomic and Simple, excluding self-hosted Jetpack sites?
  • If folks have a different category ordering in mind let me know.
  • Is domReady a decent event for waiting for all blocks/columns registration to settle? If there's something more appropriate, I can update.

Testing instructions

  • Apply D34488-code
  • Sandbox widgets.wp.com
  • Open the Block Editor from wp-admin or WordPress.com/block-editor
  • Verify that the new category order matches as expected in an Atomic and Simple Site
  • Verify that no reordering happens for Jetpack (standalone) sites

@gwwar gwwar added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 25, 2019
@gwwar gwwar requested review from simison, apeatling and a team October 25, 2019 01:22
@gwwar gwwar requested review from a team as code owners October 25, 2019 01:22
@gwwar gwwar self-assigned this Oct 25, 2019
@matticbot
Copy link
Contributor

@gwwar gwwar requested a review from mtias October 25, 2019 01:23
@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
Copy link
Member

simison commented Oct 25, 2019

would we like this to affect all site types or just Atomic and Simple, excluding self-hosted Jetpack sites?

Yeah, I think you should exclude self-hosted Jetpack sites; I don't think it's in the spirit of Gutenberg to re-organize categories for self-hosted sites but totes fine for WordPress.com which is a more curated experience.

In Jetpack we have site type utils — would that be helpful here too?

cc @jeherve @kraftbj

@kwight
Copy link
Contributor

kwight commented Oct 25, 2019

If folks have a different category ordering in mind let me know.

I would want Reusable blocks above the branding too, but that's pure bike-shedding.

@gwwar
Copy link
Contributor Author

gwwar commented Oct 25, 2019

I would want Reusable blocks above the branding too

It's not part of the category listing. I'd probably need to make a core PR

@simison
Copy link
Member

simison commented Oct 25, 2019

I would want Reusable blocks above the branding

That can get pretty complicated since it gets loaded async when you first time open the block picker. :-/

@gwwar
Copy link
Contributor Author

gwwar commented Oct 25, 2019

In Jetpack we have site type utils — would that be helpful here too?

I'm considering if we should just sort out the two common bundles for Simple/Atomic vs Jetpack first #34476 or have a temporary check/feature flag. We already do have a window object that should be able to tell the difference between environments but it's something definitely to be tidied.

🤔 I'll probably go with a simple check and we can see about sorting out bundles as part of upcoming sprints. I wouldn't want to keep it in this state for long.

@gwwar
Copy link
Contributor Author

gwwar commented Oct 25, 2019

In Jetpack we have site type utils — would that be helpful here too?

Thanks, wpcomGutenberg didn't have the props I was hoping for (though we can update that). window._currentSiteType should work provided that our hashing works for Jetpack versions to avoid this loading for older Jetpack installs.

Edit: 🤔 this might be tricky. I guess folks are downloading latest based on this issue Automattic/jetpack#12397

@simison
Copy link
Member

simison commented Oct 25, 2019

wpcomGutenberg didn't have the props I was hoping for (though we can update that). window._currentSiteType should work provided that our hashing works for Jetpack versions to avoid this loading for older Jetpack installs.

I'd looove to get those moved to wpcomGutenberg for clarity, out of the global root. Not a requirement in this PR but it would be great to get it done at some point soon. :-)

🤔 I'll probably go with a simple check and we can see about sorting out bundles as part of upcoming sprints. I wouldn't want to keep it in this state for long.

+1, sounds like a plan!

@gwwar
Copy link
Contributor Author

gwwar commented Oct 25, 2019

This one is ready for another round of 👀. I've added some temporary checks so this only runs for Simple and Atomic. D34488-code has been rebuilt with https://circleci.com/gh/Automattic/wp-calypso/475666

@gwwar
Copy link
Contributor Author

gwwar commented Oct 25, 2019

Desktop failures here are unrelated as this change doesn't touch any Calypso app bundles

@kwight
Copy link
Contributor

kwight commented Oct 25, 2019

Confirmed behaviour across all three platforms 👍

@gwwar
Copy link
Contributor Author

gwwar commented Oct 25, 2019

Thanks for the reviews @kwight @simison! I'll land this next week unless folks had any other requests for behavior/feedback. cc @apeatling if the category listing seems okay

Copy link
Member

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

Ordering looks good 👍

@gwwar gwwar merged commit 2cda95b into master Oct 28, 2019
@gwwar gwwar deleted the update/block-category branch October 28, 2019 15:40
@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 Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants