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.

Sign up for GitHub

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

Jump to bottom

Add PlanRadioButton Component #14025

Merged
merged 5 commits into from
Nov 15, 2019
Merged

Add PlanRadioButton Component #14025

merged 5 commits into from
Nov 15, 2019

Conversation

robertf4
Copy link
Contributor

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

See the P2 here for the design of this PR: p1HpG7-7MK-p2 (specifically the part titled "WP Admin Desktop")
See the P2 here for the overall MT: p1HpG7-7ET-p2

Testing instructions:

This cannot be tested on its own here. Use #14018 to test.

Proposed changelog entry for your changes:

  • Not needed

@robertf4 robertf4 requested a review from a team as a code owner November 14, 2019 05:37
@robertf4 robertf4 self-assigned this Nov 14, 2019
@robertf4 robertf4 added [Status] Needs Review To request a review from Crew. Label will be renamed soon. Admin Page React-powered dashboard under the Jetpack menu Plans and removed [Status] In Progress labels Nov 14, 2019
@robertf4 robertf4 mentioned this pull request Nov 14, 2019
Merged
delawski
delawski suggested changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

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

@robertf4 Thank you for extracting this presentational component to a separate PR. I left some feedback regarding the CSS.

I know you've got a lot on your plate right now, so if you need any support in addressing the feedback, feel free to reach out to me.

_inc/client/plans/single-product-backup.scss Outdated
@@ -0,0 +1,11 @@
.plan-radio-button__input {
margin-left: -20px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to use an !important rule here. I guess it's enough to increase the specificity of the selector by changing it to something like:

input[radio].plan-radio-button__input {

I'm wondering, though, if setting a display: flex layout on the parent element (.plan-radio-button__label) and then getting rid of the negative margin hack altogether wouldn't make more sense. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, much cleaner. Updated.

@delawski delawski added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 14, 2019
@enejb enejb force-pushed the add/products-data-component branch from ab4bdd8 to cb3d878 Compare November 14, 2019 15:04
@jeherve jeherve added this to the 8.0 milestone Nov 14, 2019
@robertf4 robertf4 changed the base branch from add/products-data-component to master November 14, 2019 19:32
@robertf4 robertf4 added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 15, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 15, 2019 •
edited

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against afc52f9

delawski
delawski approved these changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

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

Thanks for updating the code. Looks good to me now. I think it's ready to merge.

@delawski delawski added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 15, 2019
@enejb enejb merged commit f0d5168 into master Nov 15, 2019
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 15, 2019
@jeherve jeherve deleted the add/plan-radio-button branch November 15, 2019 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delawski delawski delawski approved these changes

Assignees

@robertf4 robertf4

Labels
Admin Page React-powered dashboard under the Jetpack menu Plans
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@robertf4 @jetpackbot @delawski @enejb @jeherve @matticbot