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

Custom Select: Change prop names to align better with other components. #18827

Merged
merged 1 commit into from Dec 5, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 29, 2019

Description

This PR tweaks the new CustomSelect component introduced in #17926 to better align its prop names with already existing components.

Relevant discussion: #17926 (comment).

How has this been tested?

It was verified that the font size picker and the storybook for the component still work as expected.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@epiqueras epiqueras added this to the Future milestone Nov 29, 2019
@epiqueras epiqueras self-assigned this Nov 29, 2019
@epiqueras epiqueras mentioned this pull request Nov 29, 2019
5 of 5 tasks complete
@youknowriad youknowriad requested a review from aduth Dec 2, 2019
Copy link
Contributor

youknowriad left a comment

Thanks for the changes, I'm still wondering if we should align the naming of the component with all other controls. CustomSelectControl. It makes it easy to know that this is just another control that works similarly to other controls (value, onChange, label...)?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 2, 2019

That makes sense to me. I thought Control was reserved for things that used BaseControl. I'm ok with changing it to CustomSelectControl if you and @tellthemachines agree.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 4, 2019

If this is already available in a published plugin version, we should include a temporary (2 version) deprecation for the previous prop naming.

See: https://developer.wordpress.org/block-editor/developers/backward-compatibility/deprecations/

Edit: Oh, I guess if #17926 was only merged a week ago, it's not yet been included in the plugin. Disregard, then! But we'll want to get this in before Monday.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 4, 2019

Yeah, @youknowriad @tellthemachines can we merge this before then?

@tellthemachines

This comment has been minimized.

Copy link
Contributor

tellthemachines commented Dec 5, 2019

I'm ok with changing it to CustomSelectControl if you and @tellthemachines agree.

I'm happy with that change, as long as we keep the "Custom" bit. Are you doing it in this PR?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 5, 2019

I'm not sure if we should do it to be honest.

All of the components that have the word "control" as a suffix use BaseControl. CustomSelect doesn't use BaseControl.

Also, that hasn't been applied consistently either. Some inputs use BaseControl and don't have the word "control" as a suffix.

@tellthemachines

This comment has been minimized.

Copy link
Contributor

tellthemachines commented Dec 5, 2019

All of the components that have the word "control" as a suffix use BaseControl.

I interpreted that as chance, meaning that BaseControl is just a component that you can use to build controls with. The only thing it does is output a label, and though that is a pretty basic feature for form controls, it would be more accurate to name it LabelControl. It doesn't even enforce semantics because you can use it to output a span 🤷‍♀
Also, in theory we could use BaseControl in CustomSelect because we have a label in there somewhere 😁

I'm not fussed either way. Happy to have this merged and think about the naming further elsewhere.

@epiqueras epiqueras force-pushed the update/custom-select-prop-names branch from 223a5a8 to 1a9642b Dec 5, 2019
@epiqueras epiqueras force-pushed the update/custom-select-prop-names branch from 1a9642b to 497d2ca Dec 5, 2019
@epiqueras epiqueras merged commit 6cc5811 into master Dec 5, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the update/custom-select-prop-names branch Dec 5, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 5, 2019

I still think we should rename it CustomSelectControl before the next plugin release :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.