Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

feat(layouts): add @mixin for responsive support for rows #9220

Closed
wants to merge 1 commit into from
Closed

feat(layouts): add @mixin for responsive support for rows #9220

wants to merge 1 commit into from

Conversation

clshortfuse
Copy link
Contributor

Creates @mixin that only triggers when in row configuration.

  • Dividers now properly display in dynamic layout directions.
  • Radio Buttons bottom margins are now properly removed when
    in row configuration.
  • Input, Select, Radio Buttons and Checkboxes automatically
    add 16px horizontal margin when in a row and not last item.

Previous implementations could not properly detect row configuration
where a layout would change direction based on media breakpoints.

Fixes #9112

@clshortfuse clshortfuse added ui: layout needs: review This PR is waiting on review from the team labels Aug 3, 2016
@clshortfuse
Copy link
Contributor Author

clshortfuse commented Aug 3, 2016 •
edited
Loading

I recreated this from an old PR #9112

Now the resultant CSS is actually smaller than master:

52264 (this PR) vs 52805 (master)

The auto horizontal margins can be seen with this code pen: http://codepen.io/shortfuse/pen/VjGOEx

@clshortfuse clshortfuse force-pushed the layout-whenRowMixin branch 3 times, most recently from 20ab2f0 to 5efdaff Compare August 3, 2016 20:29
@clshortfuse
Copy link
Contributor Author

Add High Density label since this is a multi-component change that reduces horizontal margins. (in other words, denser layouts)

@ThomasBurleson ThomasBurleson added this to the 1.1.2 milestone Aug 22, 2016
@clshortfuse clshortfuse modified the milestones: 1.2.0, 1.1.2 Sep 7, 2016
@ThomasBurleson ThomasBurleson modified the milestone: 1.2.0 Sep 7, 2016
@ThomasBurleson ThomasBurleson added needs: work in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Sep 7, 2016
@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Nov 22, 2017
@clshortfuse
feat(layouts): add @mixin for responsive support for rows
8f50a92 
Creates @mixin that only triggers when in row configuration.

 - Dividers now properly display in dynamic layout directions.
 - Radio Buttons bottom margins are now properly removed when
   in row configuration.
 - Input, Select, Radio Buttons and Checkboxes automatically
   add 16px horizontal margin when in a row and not last item.

Previous implementations could not properly detect row configuration
where a layout would change direction based on media breakpoints.

Fixes angular#9112
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 3, 2018
@clshortfuse
Copy link
Contributor Author

@Splaktar Rebased.

@Splaktar Splaktar self-assigned this Jan 5, 2018
@Splaktar Splaktar self-requested a review January 5, 2018 20:16
@Splaktar Splaktar added the needs: review This PR is waiting on review from the team label Jan 5, 2018
@Splaktar Splaktar removed this from the Future milestone Feb 7, 2018
@Splaktar Splaktar added this to the 1.2.0 milestone Feb 7, 2018
@Splaktar Splaktar added the P2: required Issues that must be fixed. label Feb 7, 2018
@Splaktar Splaktar removed the request for review from ThomasBurleson July 27, 2020 18:09
Splaktar
Splaktar approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

This looks good to me. It just needs to be rebased and re-tested. I'll see if I can handle it.

@Splaktar Splaktar removed the needs: review This PR is waiting on review from the team label Jul 27, 2020
Splaktar
Splaktar suggested changes Jul 27, 2020
View reviewed changes
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Actually testing this manually, I ran into some regressions. I'll open a new PR that solves those plus handles the rebase.

src/core/style/mixins.scss
// Auto insert object margin
@mixin auto-horizontal-margin($selector) {
@include when-layout-row($selector) {
&:not(last-child) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this should be :last-child to actually work properly.

That said, in my testing, that results in this:
Screen Shot 2020-07-27 at 15 00 05

Using :first-child seems to give us what we want:
Screen Shot 2020-07-27 at 15 00 34

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 think it should be :last-child as you noted, or else it would target against <last-child> tag name, which would be wrong. I would check against a row with 3 items to ensure the auto margins work. It might look right with :first-child because they're only 2 items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rule, in theory, should pad $default-horizontal-margin at the start (left on LTR). Left padding should appear on every item, except the first one, so :not(:first-child) makes sense.

Maybe I did a last minute change and had it apply margin at the end originally, and failed to adjust. So yeah, :not(:first-child) is good.

Splaktar pushed a commit that referenced this pull request Jul 27, 2020
@clshortfuse @Splaktar
feat(layouts): add @mixin for responsive support for rows
5fc0d58 
Creates `@mixin` that only triggers when in row configuration.

 - Dividers now properly display in dynamic layout directions.
 - Radio Buttons bottom margins are now properly removed when
   in row configuration.
 - Input, Select, Radio Buttons and Checkboxes automatically
   add 16px horizontal margin in front of the element
   when in a row and not the first item.

Previous implementations could not properly detect row configuration
where a layout would change direction based on media breakpoints.

Fixes #9112. Closes #9220.
Splaktar pushed a commit that referenced this pull request Jul 27, 2020
@clshortfuse @Splaktar
feat(layouts): add @mixin for responsive support for rows
32cc8e4 
Creates `@mixin` that only triggers when in row configuration.

 - Dividers now properly display in dynamic layout directions.
 - Radio Buttons bottom margins are now properly removed when
   in row configuration.
 - Input, Select, Radio Buttons and Checkboxes automatically
   add 16px horizontal margin in front of the element
   when in a row and not the first item.

Previous implementations could not properly detect row configuration
where a layout would change direction based on media breakpoints.

Fixes #9112. Closes #9220.

BREAKING CHANGE: The way that margins are applied to `md-checkbox`, `md-input-container`, `md-radio-group`, and `md-select` has been changed. You can now use the `$default-horizontal-margin` Sass variable to override the default `16px` horizontal margin size. As part of this, `md-radio-button`s inside of `layout="row"` containers are now aligned vertically with other content as they no longer have a `16px` `margin-bottom`. If you have previously added custom styles, to your components inside of a row layout, in order to give them extra `margin-right` in LTR or `margin-left` in RTL, you will need to re-evaluate those styles. In most cases, they can now be removed.
@Splaktar Splaktar mentioned this pull request Jul 27, 2020
Merged
3 tasks
@Splaktar
Copy link
Member

Continued this work in PR #11981.

Splaktar pushed a commit that referenced this pull request Jul 27, 2020
@clshortfuse @Splaktar
feat(layouts): add @mixin for responsive support for rows
c2c336b 
Creates `@mixin` that only triggers when in row configuration.

 - Dividers now properly display in dynamic layout directions.
 - Radio Buttons bottom margins are now properly removed when
   in row configuration.
 - Input, Select, Radio Buttons and Checkboxes automatically
   add 16px horizontal margin in front of the element
   when in a row and not the first item.

Previous implementations could not properly detect row configuration
where a layout would change direction based on media breakpoints.

Fixes #9112. Closes #9220.

BREAKING CHANGE: The way that margins are applied to `md-checkbox`, `md-input-container`, `md-radio-group`, and `md-select` has been changed. You can now use the `$default-horizontal-margin` Sass variable to override the default `16px` horizontal margin size. As part of this, `md-radio-button`s inside of `layout="row"` containers are now aligned vertically with other content as they no longer have a `16px` `margin-bottom`. If you have previously added custom styles, to your components inside of a row layout, in order to give them extra `margin-right` in LTR or `margin-left` in RTL, you will need to re-evaluate those styles. In most cases, they can now be removed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Splaktar Splaktar Splaktar requested changes

Assignees

@Splaktar Splaktar

Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ CSS: High Density in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs P2: required Issues that must be fixed. ui: layout
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

divider: responsive layouts are not properly supported
5 participants
@clshortfuse @Splaktar @topherfangio @ThomasBurleson @googlebot