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

Categories block: move attributes to PHP #10635

Closed
wants to merge 1 commit into from
Closed

Categories block: move attributes to PHP #10635

wants to merge 1 commit into from

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Oct 15, 2018

Description

This PR changes the code for the Categories block, per feedback and discussion on #9829. There should be no behavioral changes from master. This is essentially just a code cleanup PR.

How has this been tested?

I opened a post with an existing Categories block and checked to make sure setting alignments worked and applied the right classes in the editor and front-end.

Types of changes

  • Moved attributes definition for Categories block from its index.js to index.php to increase consistency with other blocks using server-side rendering.
  • Another benefit of the previous change is that the align attribute now uses enum to define the valid values.
  • getEditWrapperProps in index.js no longer checks the exact alignment value, as this is now covered by the attribute definition in PHP. It only checks to make sure the value is not undefined, so that an erroneous alignundefined class is never applied.
  • The Categories block is now included in test/integration/full-content/server-registered.json. I also took the opportunity to rearrange the order of the blocks listed so they are in alphabetical order.

@ZebulanStanphill
Copy link
Member Author

I have a PR to allow the Categories block to use the wide alignment planned (since the block already supports the full alignment), but it is blocked by this PR. I think it would be good to get that in for 4.1 before things start getting frozen in preparation for WP 5.0. Could this be milestoned for that version?

@chrisvanpatten chrisvanpatten requested a review from a team October 16, 2018 14:27
@chrisvanpatten chrisvanpatten added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Oct 16, 2018
@tofumatt
Copy link
Member

Just a thought: e2e tests here would really help us validate that there are no regressions from master. Would you have the chance to add tests for it? 😄

@ZebulanStanphill
Copy link
Member Author

@tofumatt I have no idea how to create tests. How do you do it, and what exactly should the e2e tests be testing?

@tofumatt
Copy link
Member

That probably means we won't have time for tests as I don't think we have time to onboard tonight 🙂 No worries!

(For what it's worth: I would want to test common category behaviour and make sure that the tests run in master and this branch with the same results.)

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Oct 16, 2018

@tofumatt Are you aware of any similar tests for other server-rendered blocks in the plugin? I might be able to squeeze in time to take a swing at it (and/or help @ZebulanStanphill) but it'd be nice to borrow from an existing example, if there is one.

(I'm also going to poke around in the test folder — not totally helpless! — but if you have an idea off the top of your head that'd be cool!)

EDIT: I'm having trouble digging up tests for any server rendered blocks. The block tests we have check against the raw block markup, not the rendered markup. We would need a way to compare with the published HTML of a post… still digging but not sure there's an example available.

@youknowriad
Copy link
Contributor

@chrisvanpatten we don't have such tests yet. I tend to prefer e2e tests have a better regression catch rate, so I'd argue we can start testing blocks using e2e tests:

  • Insert the block
  • Tweak it a bit
  • Validate that it shows up properly on the frontend
  • It doesn't have to go deep in the combinations but at least it sets a precedent for similar block tests.

@chrisvanpatten
Copy link
Contributor

@youknowriad I think if I can figure out how to snapshot the rendered markup, it should be pretty doable. Presumably we would need a getRenderedPostContent helper in utils? (I didn't see one in there.)

@ZebulanStanphill
Copy link
Member Author

Notably, I was planning on some more PRs after to this to standardize the alignments for all the widget blocks (e.g. right now the Categories block supports the full alignment but not wide for some reason), but those changes are blocked by this PR since I was trying to do changes bit-by-bit rather than all-at-once. I'm worried I'll run out of time due to the deadlines, though.

@chrisvanpatten
Copy link
Contributor

@ZebulanStanphill There's always phase 2 :)

@ZebulanStanphill
Copy link
Member Author

True, but I was hoping I could help polish the initial release just a bit more. For reference, here is the current state of alignment options on widget blocks:

  • Archives: left, center, right
  • Categories: left, center, right, full
  • Latest Comments: left, center, right, wide, full
  • Latest Posts: center (broken), wide, full

@chrisvanpatten
Copy link
Contributor

@ZebulanStanphill could you do a version of this that keeps everything identical in terms of where attributes are defined and all, but switches to use the supports: align functionality? That would contain the surface area and also ensure this block is using the latest code.

See here: https://github.com/WordPress/gutenberg/pull/10562/files#diff-b7509123419c1a7302e1331c08294262R61

You should also be able to remove the getEditWrapperProps function with this approach.

@ZebulanStanphill
Copy link
Member Author

@chrisvanpatten I thought you couldn't use supports: align with server-rendered blocks?

@chrisvanpatten
Copy link
Contributor

@ZebulanStanphill you should be able to. It only provides the UI; the server callback still needs to apply the class.

@chrisvanpatten
Copy link
Contributor

And obviously if the server callback is doing any validation of the align attribute that might need to be updated to account for the available alignments.

@ZebulanStanphill
Copy link
Member Author

@chrisvanpatten Okay, I'm testing out a new PR now. The block now lets you set any value for align via manual editing in the Code Editor, but no erroneous classes or styles are applied on the front-end or in the editor. The presence of align and the usage of enum in index.php has no effect, I'm guessing because the supports: align version overrides one set in PHP. Is this behavior alright?

@ZebulanStanphill
Copy link
Member Author

Just to clarify, I put the supports: align in JS... does it make a difference if you put it in PHP?

@ZebulanStanphill
Copy link
Member Author

Closing this PR in favor of its replacement: #10635.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants