-
Notifications
You must be signed in to change notification settings - Fork 93
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
Fix default multiselect height #1579
Conversation
e770654
to
b9d818e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
22f6f21
to
dc9de62
Compare
This comment has been minimized.
This comment has been minimized.
b9d818e
to
bfaf9dd
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.
👍 see minor comments
@@ -189,8 +198,11 @@ export default { | |||
}, | |||
|
|||
cssVars() { | |||
// Don't use margin to calculate the height if noMargin | |||
const margin = this.noMargin ? 0 : this.margin |
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.
why not take "margin='0'" as property for disabling ? probably we don't want people supplying custom margins I guess ?
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.
probably we don't want people supplying custom margins I guess ?
Yep :/
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
bfaf9dd
to
e4b04db
Compare
To review before: #1578
Switched to standard positioning to make sure the container take the content's height.
Allows design like this