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

✨Divider thickness option #2519

Merged
merged 4 commits into from
Sep 28, 2022
Merged

Conversation

oddvernes
Copy link
Collaborator

@oddvernes oddvernes commented Sep 26, 2022

resolves #2515
There was some elaborate functional code to subtract divider height from margin bottom. I am not sure why it is important the total height of the divider + margins remain the same (perhaps so the total remains divisable by 8?). Regardless I reimplemented it in 1 line of css.

Should the prop name be thickness or size, like mantine? I feel like thickness is less ambigous since the variant changes margins and therefore total size as well

Also this component does not support compact yet but I imagine this might be needed at some point

@mimarz
Copy link
Contributor

mimarz commented Sep 26, 2022

resolves #2515 There was some elaborate functional code to subtract divider height from margin bottom. I am not sure why it is important the total height of the divider + margins remain the same (perhaps so the total remains divisable by 8?). Regardless I reimplemented it in 1 line of css.

@vnys would have to answer on this 🤔

Should the prop name be thickness or size, like mantine? I feel like thickness is less ambigous since the variant changes margins and therefore total size as well

I feel like size would be more in line with the rest our components, but thickness kinda fits the theme 😅

Also this component does not support compact yet but I imagine this might be needed at some point

Compact divider would be smaller margins I guess? 🤔

Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@oddvernes oddvernes merged commit 38888e1 into develop Sep 28, 2022
@oddvernes oddvernes deleted the OOVE/2515-divider-thickness-option branch September 28, 2022 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Divider: add thickness options
2 participants