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.

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

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Fix default multiselect height #1579

merged 1 commit into from
Nov 26, 2020

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Nov 18, 2020

To review before: #1578

Switched to standard positioning to make sure the container take the content's height.
Allows design like this

Peek 26-11-2020 09-38

@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews design Design, UX, interface and interaction design feature: select Related to the NcSelect* components labels Nov 18, 2020
@skjnldsv skjnldsv self-assigned this Nov 18, 2020
@skjnldsv skjnldsv force-pushed the fix/multiselect-height branch 3 times, most recently from e770654 to b9d818e Compare November 18, 2020 09:16
@PVince81

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@PVince81

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv force-pushed the feat/ListItemIcon branch 3 times, most recently from 22f6f21 to dc9de62 Compare November 26, 2020 08:03
Base automatically changed from feat/ListItemIcon to master November 26, 2020 08:23
@PVince81

This comment has been minimized.

@skjnldsv
Copy link
Contributor Author

Peek 26-11-2020 09-38
Rebased and fixed Multiselect inner height (with docs)

Copy link
Contributor

@PVince81 PVince81 left a 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
Copy link
Contributor

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 ?

Copy link
Contributor Author

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 :/

src/components/Multiselect/Multiselect.vue Outdated Show resolved Hide resolved
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv reopened this Nov 26, 2020
@skjnldsv skjnldsv merged commit 5b8a08b into master Nov 26, 2020
@skjnldsv skjnldsv deleted the fix/multiselect-height branch November 26, 2020 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Design, UX, interface and interaction design feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants