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

fix radius on input-group-label and input-group-button #11104

Merged
merged 4 commits into from
Jul 2, 2018
Merged

fix radius on input-group-label and input-group-button #11104

merged 4 commits into from
Jul 2, 2018

Conversation

andycochran
Copy link
Contributor

This PR closes #8833 by explicitly setting styles for .input-group-button within .input-group so that it and .input-group-label have the expected border radius.

@andycochran andycochran requested a review from ncoden March 28, 2018 17:41
ncoden
ncoden suggested changes Mar 28, 2018
View reviewed changes
Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

Three things to keep in mind when adding selectors:

  • Selectors specificity must be the lowest possbile
  • Parents should not apply properties automatically on their children
  • Duplicated properties

scss/forms/_input-group.scss Outdated
border-radius: if($global-text-direction == rtl, 0 $input-radius $input-radius 0, $input-radius 0 0 $input-radius);
}

&.input-group-button > * {
Copy link
Contributor

Choose a reason for hiding this comment

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

Selectors can be merged here are the code in the same. So:

&:not(.input-group-button), &.input-group-button > * {
   ...
}

(don't do that, see the comment above).

Otherwise we're just duplicating code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've DRY'd this. :)

scss/forms/_input-group.scss Outdated
@@ -34,11 +34,21 @@ $input-prefix-padding: 1rem !default;
}

> :first-child {
border-radius: if($global-text-direction == rtl, 0 $input-radius $input-radius 0, $input-radius 0 0 $input-radius);
&:not(.input-group-button){
Copy link
Contributor

Choose a reason for hiding this comment

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

Foundation provide a SASS library, not only CSS components.

  • We can't expect Input group button to be implemented as .input-group-button (user should be able to create custom components with input group mixins)
  • Why only targetting Input group button ?

So I would rather go for:

&, &  > * { ... }

To be honnest, the > * is ugly and should be removed. But we'll keep it for backward compatibility. Adding a comment here like "don't do this" could be useful so it does not became an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since .input-group-button is hardcoded later in the foundation-form-prepostfix mixin, can't we expect it to be implemented as .input-group-button?

Copy link
Contributor

@ncoden ncoden Mar 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah f**. I did not realized that Input Group was not component-oriented at all !
Well, its still a problem: Why only targetting Input group button ?

This is generally a bad idea to make our CSS component having particular effects on each-others as the user should be able to use them with custom CSS alongside its own components. Input Group should have the same behavior no matter what is the class of the button we put it. This is a quiet easy way to make someone loose 1 hour: "Why when I put my own button there it is not rounded anymore."

@andycochran
Copy link
Contributor Author

@ncoden I say we live with this not being component-oriented, and make sure not to repeat this mistake in v7. Are there any remaining changes you require?

@ncoden
Copy link
Contributor

ncoden commented Mar 28, 2018

Why only targetting Input group buttons ?

@andycochran
Copy link
Contributor Author

Because the docs explicitly state to place buttons inside the input-group-button wrapper.

@andycochran
Copy link
Contributor Author

We could probably get rid of the > * too:
&.input-group-button .button

@ncoden
Copy link
Contributor

ncoden commented Mar 30, 2018 •
edited
Loading

We should assume as little as possible to HTML structure inside our components. People should be able to freely use the classes of components/elements we provide without limited by self-references and dependencies (like .button in a selector that have nothing to do with our Button components).

Because the docs explicitly state to place buttons inside the input-group-button wrapper.

Got it. So I think we can drop &:not(.input-group-button) and use:

&, &.input-group-button > * { ... }

As .input-group-button does not have any visual properties.

@ncoden
Copy link
Contributor

ncoden commented Apr 9, 2018

@andycochran What do you think ?

@ncoden
Copy link
Contributor

ncoden commented Apr 22, 2018

@andycochran Hey 👋. Will you have some time work on this ? I'd like your opinon on #11104 (comment)

@ncoden
refactor: simplify Input Group border-radius selector
d267df4 
We should assume as little as possible to HTML structure inside our components. People should be able to freely use the classes of components/elements we provide without limited by self-references and dependencies.

This commit simplify the selector used to apply border-radius on Input Group childreens to make all childreens affected, with a single exception for `.input-group-button` where sub-childreens are affected too.
@ncoden ncoden added this to the 6.5.0 milestone Jul 1, 2018
@ncoden
Copy link
Contributor

ncoden commented Jul 1, 2018

I applied the requested changes

We should assume as little as possible to HTML structure inside our components. People should be able to freely use the classes of components/elements we provide without limited by self-references and dependencies.

This commit simplify the selector used to apply border-radius on Input Group childreens to make all childreens affected, with a single exception for .input-group-button where sub-childreens are affected too.

Looks ready for stable v6.5.0

@DanielRuf
Copy link
Contributor

Oh, node-sass failing on Node.js 10?

@ncoden ncoden merged commit 8a10879 into develop Jul 2, 2018
@ncoden ncoden deleted the andycochran/8833-input-group-label-radius branch July 2, 2018 20:27
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jul 6, 2018
@ncoden
Use pull request foundation#11104 from zurb/andycochran/8833-input-gr…
bea1e77 
…oup-label-radius for v6.5.0

8081d51 fix radius on input-group-label and input-group-button
713b541 dry input-group scss
d267df4 refactor: simplify Input Group border-radius selector

Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jul 8, 2018
@ncoden
Use pull request foundation#11104 from zurb/andycochran/8833-input-gr…
1080d97 
…oup-label-radius for v6.5.0

8081d51 fix radius on input-group-label and input-group-button
713b541 dry input-group scss
d267df4 refactor: simplify Input Group border-radius selector

Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
@ncoden ncoden mentioned this pull request Jul 8, 2018
Merged
10 tasks
This was referenced Apr 22, 2020
Open
Open
Merged
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncoden ncoden ncoden approved these changes

Assignees
No one assigned
Labels
bug fix forms
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.

Fix input-group-label's border-radius when input-group-label is the last child
3 participants
@andycochran @ncoden @DanielRuf