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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Action Buttons displaying tiny icons #1042

Closed
deloreyj opened this issue Sep 1, 2020 · 7 comments 路 Fixed by #1058
Closed

Action Buttons displaying tiny icons #1042

deloreyj opened this issue Sep 1, 2020 · 7 comments 路 Fixed by #1058
Labels
bug Something isn't working good first issue Good for newcomers rsp:ActionButton

Comments

@deloreyj
Copy link
Contributor

deloreyj commented Sep 1, 2020

馃悰 Bug Report

When used on a site that sets box-sizing with the univeral box sizing with inheritance approach, the icons inside of action buttons appear tiny.

html {
  box-sizing: border-box;
}
*, *:before, *:after {
  box-sizing: inherit;
}

馃 Expected Behavior

Icons should appear normal size regardless of global box sizing rules
image

馃槸 Current Behavior

Icon appears tiny inside action button
image

馃拋 Possible Solution

It appears to be happening because the inside the action button does not define a box-sizing rule, which typically works because it expects the default content-box. One solution would be to add a style rule like the following to prevent the svg from inheriting

.ActionButton svg {
  box-sizing: initial
}

or

.ActionButton svg {
  box-sizing: unset
}

馃敠 Context

Magento runs react-spectrum UI inside the Magento shell and the shell CSS causes this bug in all our apps

馃捇 Code Sample

https://github.com/deloreyj/spectrum-action-button/tree/master

馃Б Your Company/Team

Adobe Magento

@evargast
Copy link
Contributor

evargast commented Sep 2, 2020

Just dropping a comment here to follow this thread. Stumbled upon the same situation just now! :)

@devongovett
Copy link
Member

@deloreyj I cloned your report into a code sandbox and the issue doesn't seem to occur. https://codesandbox.io/s/eloquent-wildflower-9jsn7 I did have to change @mage-catfish/react-script to the normal react-scripts though. Perhaps something in that?

@evargast do you have an example?

@evargast
Copy link
Contributor

evargast commented Sep 2, 2020

@devongovett

Sure thing!

You can reproduce it on storybook (npm run storybook) or just standalone (npm run start)
https://github.com/evargast/tiny-icons

Live Storybook for it: https://evargast.github.io/tiny-icons/

@devongovett
Copy link
Member

Ah, so setting * { box-sizing: border-box } seems to break it. I think I missed that in the original issue. I suppose we could override in Spectrum CSS to set box-sizing: content-box on the icon, but I imagine that isn't the only place we'd need to do that. Is there a reason why you're setting it on every element rather than individual elements in your own components?

@devongovett devongovett added bug Something isn't working good first issue Good for newcomers labels Sep 2, 2020
@deloreyj
Copy link
Contributor Author

deloreyj commented Sep 3, 2020

The specific answer is we don't own the html template that our react spectrum app gets rendered inside.

More generally, it's fairly common practice for sites built pre flex/grid to set box-sizing to border-box globally to make layout more intuitive. If I thought this bug would be isolated to a weird decision made in the Magento codebase (of which there are many), I'd have just patched it and moved on. But I suspect a lot of sites with legacy baggage may run into this issue.

@devongovett
Copy link
Member

馃憤 makes sense. We can patch this in the button css here:

.spectrum-Icon {
max-block-size: 100%;
flex-shrink: 0;
order: 0; /* always be before the label, regardless of DOM order */
transition: background var(--spectrum-global-animation-duration-100) ease-out,
fill var(--spectrum-global-animation-duration-100) ease-out;
}

Feel free to submit a PR to help move this along! 馃檱

@deloreyj
Copy link
Contributor Author

deloreyj commented Sep 4, 2020

Will do, thanks for the direction!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers rsp:ActionButton
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants