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

Framework: Remove deprecations slated for 3.4 removal #8276

Merged
merged 7 commits into from
Jul 31, 2018

Conversation

pento
Copy link
Member

@pento pento commented Jul 30, 2018

This PR removes deprecations scheduled to be removed in the upcoming 3.4.0 release.

  • focusOnMount prop in the Popover component has been changed from Boolean-only to an enum-style property that accepts "firstElement", "container", or false. Please convert any <Popover focusOnMount /> usage to <Popover focusOnMount="firstElement" />.
  • wp.utils.keycodes utilities are removed. Please use wp.keycodes instead.
  • Block id prop in edit function removed. Please use block clientId prop instead.
  • property source removed. Please use equivalent text, html, or attribute source, or comment attribute instead.

Source: https://wordpress.org/gutenberg/handbook/reference/deprecated/#3-4-0

@pento pento added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jul 30, 2018
@pento pento added this to the 3.4 milestone Jul 30, 2018
@pento pento requested a review from a team July 30, 2018 04:37
@pento
Copy link
Member Author

pento commented Jul 30, 2018

Note: the id prop is still actually being passed to edit, as this comment says it's going to be removed in 3.5:

// TODO: `id` prop is to be removed in 3.5 "UID" deprecation.
return (
<Component
{ ...props }
id={ props.clientId }
className={ className }
/>
);
};

// Backcompat for Color Palette set through `gutenberg` array.
if ( empty( $color_palette ) && ! empty( $gutenberg_theme_support[0]['colors'] ) ) {
$color_palette = $gutenberg_theme_support[0]['colors'];
}

if ( ! empty( $gutenberg_theme_support ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

@jorgefilipecosta, I think I missed something here, can you help?

Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 30, 2018

Choose a reason for hiding this comment

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

Hi @gziolo, this code is to keep compatibility with the old Gutenberg theme support. The code block // Backcompat for Color Palette set through "gutenberg" array. should also be removed.
I also did a checkup on the logic and we had missed a removal in a previous version so I removed that code. I added my changes in a new commit 417203c.

@gziolo
Copy link
Member

gziolo commented Jul 30, 2018

I added a commit with a code removal for PHP part. I think I did something wrong though. @jorgefilipecosta @pento can you do sanity check?

@aduth
Copy link
Member

aduth commented Jul 30, 2018

Note: the id prop is still actually being passed to edit, as this comment says it's going to be removed in 3.5:

The block id prop was deprecated in a version prior to the bulk of the uid -> clientId renaming, so the comment isn't accurate (said as the person who likely authored it).

I think we should remove it now, consistent with our deprecated messaging.

@@ -57,8 +57,6 @@ By default, the *first tabblable element* in the popover will receive focus when

Set this prop to `false` to disable focus changing entirely. This should only be set when an appropriately accessible substitute behavior exists.

**Deprecation notice:** Before Gutenberg 3.2 this value was `Boolean` and the value `true` was equivalent to `"firstElement"`. This behaviour is deprecated and will cause a console warning message.
Copy link
Member

Choose a reason for hiding this comment

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

Anecdote: I only stumbled upon this by chance. Fewer touch points with deprecations is better, ideally colocated with the deprecated function call which is the target of our linting rules.

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.

E2E tests are struggling with unrelated issues, but otherwise this is looking good.

@aduth aduth force-pushed the remove/3-4-deprecations branch from 5e016f7 to 3d769e1 Compare July 30, 2018 18:15
@aduth
Copy link
Member

aduth commented Jul 30, 2018

  • Rebased to resolve conflicts.
  • Removed id prop passing to edit in 81dce98
  • Updated Freeform block, which apparently still depended on props.id, to use props.clientId

The last of these could do for a bit of extra testing / observing whether other blocks have similar deprecated usage.

@pento
Copy link
Member Author

pento commented Jul 31, 2018

Changes are looking good, @aduth. No apparent problems, and I couldn't find any other blocks that appeared to be using the id prop. (An exception is GalleryImage, which is explicitly passed a prop called id.)

I ran across a fun little issue that was caused by id changing to clientId: React doesn't like when you pass a camelCase prop to a HTML element that it doesn't know what it should do with. Also reported in #8136.

@pento pento merged commit 656439c into master Jul 31, 2018
@pento pento deleted the remove/3-4-deprecations branch July 31, 2018 05:43
@aduth
Copy link
Member

aduth commented Jul 31, 2018

I ran across a fun little issue that was caused by id changing to clientId: React doesn't like when you pass a camelCase prop to a HTML element that it doesn't know what it should do with. Also reported in #8136.

This sounds like it's always been a buggy implementation, if it was blindly applying the props when said props include block-specific values. It's also why ...props spread application on elements can be discouraged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants