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

Introduce prepend_to_selector() to avoid additional if checks and follow single responsibility principle #50266

Merged
merged 4 commits into from
May 23, 2023

Conversation

felixarntz
Copy link
Member

What?

This implements the WP core PR WordPress/wordpress-develop#4380 in Gutenberg first, as requested on that PR, which makes sense given the current Gutenberg implementation differs slightly from current WP core's.

Why?

Having to check on every append_to_selector() call what the append position is inefficient and violates the single responsibility principle. The code that calls this method already knows whether it needs to append or prepend, so having a separate prepend_to_selector() method with only that responsibility is more efficient.

Related: #47833

@github-actions
Copy link

github-actions bot commented May 2, 2023

Flaky tests detected in 45473d9.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4866147232
📝 Reported issues:

*
* Given the compounded $selector "h1, h2, h3"
* and the $to_prepend selector ".some-class " the result will be
* ".some-class h1, .some-class h2, .some-class h3".
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted in a Slack DM to @felixarntz:

A $to_prepend of ".some-class " will produce: .some-class h1,.some-class h2,.some-class h3.

This was the result prior to this PR though. See this 3v4l.

One way to achieve the result in the docblock is trim( $sel ) and implode( ', ', $new_selectors ). See this 3v4l.

I'm not sure if these "misplaced" spaces were already known and accepted, or if this is something tests should have caught.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @costdev, good catch!

We could add trim() to the function, but to me that seems like a separate conversation. The output with trim would be preferable, but it's also not a big deal to have the extra space, and since the function is called tons of times there may be some (probably insignificant) performance cost to it.

In other words, I'm by no means opposed, but potentially this could be discussed in a separate issue / PR.

I have fixed the doc block to include the double spaces to clarify that.

Copy link
Contributor

@costdev costdev May 4, 2023

Choose a reason for hiding this comment

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

Sounds like a plan @felixarntz!

For a separate issue / PR, as an alternative to the trim(), if we know that the format always includes a space after the comma, we could explode( ', ', $selector ) and implode( ', ', $selector ). See this 3v4l.

@spacedmonkey
Copy link
Member

@felixarntz Coding standards failure needed to be fixed.

@spacedmonkey
Copy link
Member

Note: we should backport this - #47833 as the same time.

@spacedmonkey spacedmonkey merged commit 58a22ce into trunk May 23, 2023
@spacedmonkey spacedmonkey deleted the enhance/append-to-selector-performance branch May 23, 2023 12:35
@github-actions github-actions bot added this to the Gutenberg 15.9 milestone May 23, 2023
@cbravobernal cbravobernal added the [Type] Code Quality Issues or PRs that relate to code quality label May 24, 2023
@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jun 9, 2023
@ramonjd ramonjd removed the Needs PHP backport Needs PHP backport to Core label Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants