-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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 list element feature [Fixes #772] #1720
Conversation
You forgot to remove Otherwise, looks good to me. |
Updated. |
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.
Great first PR! You followed the guidelines. Some changes required though.
sass/components/list.sass
Outdated
background-color: $list-background-color | ||
border: $list-border | ||
border-radius: $list-radius | ||
.list-item |
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 wouldn't nest this under .list
.
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.
where then?
sass/components/list.sass
Outdated
border: $list-border | ||
border-radius: $list-radius | ||
.list-item | ||
color: black |
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.
Use color: $list-item-color
and set the default value to $text
.
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.
oh yes, my fault :O
sass/components/list.sass
Outdated
border-radius: $list-radius | ||
.list-item | ||
color: black | ||
cursor: 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.
Remove this.
sass/components/list.sass
Outdated
.list-item | ||
color: black | ||
cursor: default | ||
display: flex |
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.
Remove this.
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.
sass/components/list.sass
Outdated
color: black | ||
cursor: default | ||
display: flex | ||
justify-content: flex-start |
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.
Remove this.
sass/components/list.sass
Outdated
cursor: default | ||
display: flex | ||
justify-content: flex-start | ||
padding: 8px 16px |
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.
Use padding: 0.5em 1em
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 didn't understand em and rem until today xD
sass/components/list.sass
Outdated
padding: 8px 16px | ||
&:not(:last-child) | ||
border-bottom: $list-item-border | ||
& .is-hoverable:hover |
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 like this idea. But the syntax is &.is-hoverable:hover
(without the space) and should be nested under .list-item
.
Also, can you add a &.is-active
state?
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.
which color should it have? i tried to give it primary but then the text color is still black
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 more improvements?
sass/components/list.sass
Outdated
.list-item | ||
color: black | ||
cursor: default | ||
display: flex |
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.
sass/components/list.sass
Outdated
padding: 8px 16px | ||
&:not(:last-child) | ||
border-bottom: $list-item-border | ||
& .is-hoverable:hover |
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.
which color should it have? i tried to give it primary but then the text color is still black
With the current implementation card appears to be slightly wider then the list because of border instead of box shadow. Switching to box shadow fixes this and, in my opinion, gives a nicer feel to it: .list
background-color: $list-background-color
- border: $list-border
+ box-shadow: $card-shadow
border-radius: $list-radius
&.is-hoverable > .list-item:hover:not(.is-active)
background-color: $list-item-hover-color
cursor: pointer |
Also, one of my use-cases for this list would be either a key value list or a list of labels and icons: <div class="list">
<div class="list-item">
<div class="level">
<div class="level-left">
First
</div>
<div class="level-right">
<i class="fa fa-check fa-fw"></i>
</div>
</div>
</div>
</div> But because of the What would be the workaround for that? |
eventually |
Yeah, that's how I did it for the screenshot, but I thought the flex display was there for a reason. |
What does @jgthms think about the border? |
The shadow is a good shout. Let’s put that instead of the border. |
|
In my opinion
|
sass/components/list.sass
Outdated
.list-item | ||
color: $list-item-color | ||
display: block | ||
padding: 0.375rem 0.75rem |
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.
You need padding: 0.5em 1em
here.
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.
@jgthms Why is the 0.5em 1em
padding better than 0.375rem 0.75rem
?
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.
Uses em
so it's proportional to the list's font size, and just looks better.
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 get the second part, but don't understand how do you decide whether to use rem
or em
.
For vertical margins(level, card, panel) you tend to use rem
but sometimes(form label) you use em
.
For horizontal margins you tend to use em
but sometimes(tags) you use rem
.
Similarly for paddings.
What's your rule of thumb when it comes to these units?
I feel like I'm being annoying but I just want to understand your train of though.
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.
Margin is between elements, hence a common denominator like rem. Padding is within an element and the visual balance is assured when everything is proportional to the font size hence em.
Button padding is em. Same for form controls.
sass/components/list.sass
Outdated
|
||
$list-item-border: 1px solid $border !default | ||
$list-item-color: $text !default | ||
$list-item-active-color: $primary !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.
$list-item-active-background-color: $link !default
sass/components/list.sass
Outdated
$list-item-border: 1px solid $border !default | ||
$list-item-color: $text !default | ||
$list-item-active-color: $primary !default | ||
$list-item-active-text-color: $white !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.
$list-item-active-color: $link-invert !default
sass/components/list.sass
Outdated
$list-item-color: $text !default | ||
$list-item-active-color: $primary !default | ||
$list-item-active-text-color: $white !default | ||
$list-item-hover-color: $background !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.
$list-item-hover-background-color: $background !default
Ready to merge? |
sass/components/list.sass
Outdated
|
||
.list | ||
background-color: $list-background-color | ||
box-shadow: $card-shadow |
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.
Don’t reference a variable from another file. Create a new one.
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.
xD
Feedback? |
push |
Any update? Will this be merged? |
Any updates ? I think this feature would be very usefull |
@jgthms can you please merge? Is there another collaborator who can do it for you? |
@danieldaeschle can you update docs? |
My english is notj good enough to write fancy docs. i can do it but now yet and only if @jgthms says he will merge it then. |
Do what you can and tag us, we'll help you out. |
Thanks, but I'm waiting for confirmation from @jgthms |
Wow, thanks :) |
No, thank you! 😉 |
@danieldaeschle any docs? How to use? With english we can help. |
@frederikhors Not much to it: <div class="list">
<div class="list-item">First</div>
<div class="list-item is-active">Second</div>
<div class="list-item">Third</div>
</div> If you want the items behaving like links: <div class="list is-hoverable">
<div class="list-item">First</div>
<div class="list-item is-active">Second</div>
<div class="list-item">Third</div>
</div> |
Link of list is missing at components, i try with url and it works. |
This is a new feature.
How to update the docs/changelog?
Proposed solution
It adds a list element which was missing of the most of users
Testing Done
I checked in browser how it looks