-
Notifications
You must be signed in to change notification settings - Fork 166
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
Add equal height row component #5039
Conversation
A rough initial setup to get a sense for what needs to be implemented. There are still some discussions to be had about the approach. |
79f17c2
to
a018a27
Compare
5a838b0
to
f41463c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, some comments in the code
scss/_patterns_equal-height-row.scss
Outdated
.p-equal-height-col { | ||
display: grid; | ||
grid-column: span calc($grid-columns / 4); | ||
grid-row: span 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10? wasn't there an auto value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be auto because .p-equal-height-col
is a subgrid and its parent grid has grid-template-rows
set to default auto. This means the subgrid is actually defining the row structure for the parent grid, so we need to explicitly define the row span. Although we can set the span to 4 instead of 10 to cater for maximum 4 grid items, previously this is set to 10 to cater for an arbitrary number of grid items in a column, but we have decided to have maximum 4 items in a column now.
templates/docs/examples/patterns/equal-height-row/4-item-columns.html
Outdated
Show resolved
Hide resolved
{% block content %} | ||
|
||
<div class="row"> | ||
<div class='p-equal-height-row has-divider-below-item-1'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iwould refer to the dividers as "above". They are attached top the element that follows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use "above", then the dividers will have to start at item-2 i.e. has-divider-above-item-2
, has-divider-above-item-3
, has-divider-above-item-4
. Seems a little weird to start at item-2 for me, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In separate discussion with bartek, we think has-1st-divider
, has-2nd-divider
and has-3rd-divider
might be a good compromise to make the classes less verbose, wdyt?
82d9f37
to
a1bdf81
Compare
scss/_patterns_equal-height-row.scss
Outdated
} | ||
|
||
// For smaller screens, the divider pseudo elements are inserted at the column level | ||
&.p-equal-height-row--divider-below-item-1 .p-equal-height-row__col::before, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use actual border on small screens instead of the before/after pseudo element divider?
So, if we have a class name that adds divider between items 1/2 - the item 2 will have border on top:
// pseudo-code, just to give idea
&.has-divider-above-item-2 .p-equal-height-row__item:nth-child(2) {
border-top: 1px solid $colors--theme--border-default;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try with border first, but it doesn't play nicely with spacing such as margin-top
. Using pseudo elements, the absolute positioning inside the grid works for all kinds of spacing between items since the element aligns exactly with the grid tracks.
5b558ab
to
cd13e14
Compare
0ec409f
to
a5b3b4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the docs contents.
5be756a
to
fddcd26
Compare
assets for thew above 4 images: https://assets.ubuntu.com/manager?tag=equal-heights-component-example-assets |
fddcd26
to
0bff7ef
Compare
@lyubomir-popov thanks I have replaced the images |
@mas-who Could you also add |
0bff7ef
to
937f42e
Compare
d4c986f
to
c3a244b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestions for the docs
c3a244b
to
89d4c7d
Compare
- Added equal height grid row pattern with full screen sized dividers Signed-off-by: Mason Hu <mason.hu@canonical.com>
89d4c7d
to
72c6592
Compare
Really nice work @mas-who +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks @mas-who !
Done
Added the equal height row component which includes the following features:
Fixes: WD-10102
QA
Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots