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 Library: Fix archive block classes (#26349) #26364

Closed
wants to merge 1 commit into from
Closed

Block Library: Fix archive block classes (#26349) #26364

wants to merge 1 commit into from

Conversation

fklein-lu
Copy link
Contributor

Description

Issue #26349 describes how Archive blocks have an extra space before the first class. So currently the output is class=" wp-block-archives-list wp-block-archives".

By using an array for constructing classes instead of string concatenation, this issue can be avoided.

This PR also fixes the escaping to always be late, and adds PHP unit tests for good measure.

How has this been tested?

  • PHP unit test.
  • Manual test:
    • Open the post editor and add an Archives block.
    • View the post on the frontend and note the classes have no prepended space anymore.

Types of changes

Bug fix, no breaking changes.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@fullofcaffeine
Copy link
Member

This seems to work well! I tested one of the problematic blocks with Gutenberg 9.2.0 and after applying this fix, the class doesn't have the leading space anymore.

Vanilla GB 9.2.0 This branch
beforefix fixapplied

I don't have the permissions to approve this PR, and I'd rather leave the final decision to one of the core devs, but looks good so far.

@ockham
Copy link
Contributor

ockham commented Oct 22, 2020

Possibly fixed by #26309?

@ockham
Copy link
Contributor

ockham commented Oct 22, 2020

Possibly fixed by #26309?

Confirming that the tests added by this PR pass if run against master (with #26309 merged).

@ockham
Copy link
Contributor

ockham commented Oct 22, 2020

Possibly fixed by #26309?

Confirming that the tests added by this PR pass if run against master (with #26309 merged).

Interestingly, I still see a leading space when running tests on our (closed) CI system: #26309 (comment) 🤔

@gziolo gziolo added [Package] Block library /packages/block-library [Block] Archives Affects the Archives Block [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Oct 28, 2020
@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

@fklein-lu, can you rebase this PR. It looks like the block's render_callback was fixed as @ockham mentioned but we would still benefit from having this unit test added.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the php unit tests, but this seems fine to me. Thanks!

Base automatically changed from master to trunk March 1, 2021 15:44
@Mamaduka
Copy link
Member

Mamaduka commented Sep 7, 2021

Hi, @fklein-lu

Can you rebase this PR? I think we're good to merge.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Archives Affects the Archives Block [Package] Block library /packages/block-library [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants