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

[@mantine/core] ButtonGroup: Fix overlap between buttons #1979

Merged
merged 5 commits into from Aug 12, 2022
Merged

[@mantine/core] ButtonGroup: Fix overlap between buttons #1979

merged 5 commits into from Aug 12, 2022

Conversation

ryuhangyeong
Copy link
Contributor

@ryuhangyeong ryuhangyeong commented Aug 3, 2022

Example

Before

image

After

image

image

Suggest

I think it will look better if you fix the area between the button groups.

@rtivital
Copy link
Member

rtivital commented Aug 5, 2022

It will make border thinner than expected on screens with high dpi

Снимок экрана 2022-08-05 в 20 15 17

@ryuhangyeong
Copy link
Contributor Author

ryuhangyeong commented Aug 6, 2022

I didn't realize that the screen I was looking at was of low spec.

How about using the formula below? Similar to the button border, I think you can change the size of the border between them.

'& + [data-button]': {
  [orientation === 'vertical' ? 'marginTop' : 'marginLeft']: -buttonBorderWidth / 2, // here
},

@rtivital
Copy link
Member

rtivital commented Aug 6, 2022

Yeah, that might work for screens with high dpi, but I'm not sure if it will work correctly for regular screens. I've not explored much but we might need a media query to handle that – https://stackoverflow.com/a/58361747

@ryuhangyeong
Copy link
Contributor Author

image

Strangely, the border size is not constant. I don't know if I can provide a consistent style through media queries.

@rtivital
Copy link
Member

rtivital commented Aug 7, 2022

I think it should be 0.5 for high dpi and 1 for low dpi

@ryuhangyeong
Copy link
Contributor Author

Is the code below correct what you want? The standard for high resolution is 300 dpi.

'& + [data-button]': {
  [orientation === 'vertical' ? 'marginTop' : 'marginLeft']: -buttonBorderWidth,
  '@media only screen and (min-resolution: 300dpi)': {
    [orientation === 'vertical' ? 'marginTop' : 'marginLeft']: -buttonBorderWidth / 2,
  },
},

@rtivital
Copy link
Member

rtivital commented Aug 7, 2022

Not sure yet, let's have a look if it works correctly after you update a PR

@ryuhangyeong
Copy link
Contributor Author

ryuhangyeong commented Aug 7, 2022

image
image

In my environment it looks like a photo. Can I check it in high definition? Code reflected in PR.

@rtivital
Copy link
Member

rtivital commented Aug 7, 2022

I've updated media query to work correctly for high dpi, can you check that it works for low dpi?

@ryuhangyeong
Copy link
Contributor Author

I'm a little confused as to whether I understood and tested what you are saying correctly. 🤔

this is the result of testing under the following conditions after lowering the dpi as much as possible in my Windows
environment.

buttonBorderWidth: >= 4

image

buttonBorderWidth: < 4

image

Is what I tested correct what you want??

@rtivital
Copy link
Member

rtivital commented Aug 7, 2022

Seems to be correct, thanks!

@rtivital rtivital merged commit f82e20c into mantinedev:master Aug 12, 2022
@rtivital
Copy link
Member

Thanks again, @ryuhangyeong!

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.

None yet

2 participants