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

Restore priority on embed block for raw transforming #6572

Merged
merged 2 commits into from
May 3, 2018

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 3, 2018

Description

In #5966 I was so stupid to remove the priority from the paragraph block and not testing embeds... This restores the priority and transform finder.

How has this been tested?

Ensure pasting a URL in an empty paragraph embeds.

Screenshots

Types of changes

Checklist:

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

@ellatrix ellatrix added this to the 2.8 milestone May 3, 2018
@ellatrix ellatrix requested review from mtias and aduth May 3, 2018 16:09
@ellatrix ellatrix force-pushed the fix/paste-url-embed-priority branch from 741ef16 to 6797002 Compare May 3, 2018 16:10
@@ -271,6 +271,7 @@ export const settings = getEmbedBlockSettings( {
from: [
{
type: 'raw',
priority: 0,
Copy link
Member

Choose a reason for hiding this comment

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

Does priority need to be so low? While negatives are supported, I'd tend to think of zero as the lowest possible. I'm wondering if instead we should just increase the priority value for paragraph, so that it's properly treated as a fallback if no others exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The problem with adding it to paragraph is that it's not for paragraph, it's for the embed block. So this would need a comment, while here it's obvious.
  2. The priority on the paragraph was 20, which was also random? What's the default? 10? S this should be 9?

Copy link
Member

Choose a reason for hiding this comment

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

The problem with adding it to paragraph is that it's not for paragraph, it's for the embed block.

Adding it to paragraph is to reflect that paragraph is ultimately the fallback, and anything else should take precedent over it, without needing to express its own priority. It may not be obvious to plugins that they'd need to assign priority, for example, and transforms might prefer paragraph when omitted.

The priority on the paragraph was 20, which was also random? What's the default? 10? S this should be 9?

The numeric priorities are certainly imperfect 😄 9 would reflect that we want it to take place before the baseline (if paragraph's considered the baseline), while still allowing others to be considered as more important. 0 is fine enough I guess, but leaves the impression that we'd never want anything else to take precedence over the embed's transform.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, makes sense. I'll change it.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

@ellatrix ellatrix merged commit 853d4c0 into master May 3, 2018
@ellatrix ellatrix deleted the fix/paste-url-embed-priority branch May 3, 2018 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants