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.

Sign up for GitHub

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

Jump to bottom

box-sizing styling affects non-amplify elements #1092

Closed
2 tasks done
carlton-bennett opened this issue Jan 5, 2022 · 5 comments · Fixed by #1151
Closed
2 tasks done

box-sizing styling affects non-amplify elements #1092

carlton-bennett opened this issue Jan 5, 2022 · 5 comments · Fixed by #1151
Assignees
@reesscot
Labels
React An issue or a feature-request for React platform

Comments

@carlton-bennett
Copy link

Before creating a new issue, please confirm:

On which framework/platform are you having an issue?

React

Which UI component?

Other

How is your app built?

Create React App

Please describe your bug.

This rule in the CSS for amplify-ui affects other non-amplify elements on the page.

What's the expected behaviour?

The styles included with amplify-ui should be scoped such that they don't affect non-amplify elements on the page.

Help us reproduce the bug!

Create a new app using the code snippet below. The box-sizing of the blue section is set to content-box in the CSS. The rule from amplify-ui overrides this to be border-box. Clicking the toggle button will toggle an inline style which sets it back to content-box. The div changes size to illustrate the real-world impact of this rule.

Code Snippet

// index.css
section {
  box-sizing: content-box;
  background-color: blue;
  width: 500px;
  height: 100px;
  padding: 50px;
}
// index.tsx
import { AmplifyProvider } from '@aws-amplify/ui-react';
import React, { useState } from 'react';
import ReactDOM from 'react-dom';

import '@aws-amplify/ui-react/dist/index.css';
import './index.css';


function App() {
    const [ force, setForce ] = useState(false);

    return (
        <AmplifyProvider>
            <section
                style={!force ? undefined : {
                    boxSizing: 'content-box'
                }}
            />

            <button onClick={() => setForce(!force)}>
                toggle
            </button>
        </AmplifyProvider>
    );
}

ReactDOM.render(
    <App />,
    document.getElementById('root')
);

Additional information and screenshots

No response
@reesscot reesscot added React An issue or a feature-request for React platform pending-triage Issue is pending triage labels Jan 5, 2022
@reesscot
Copy link
Contributor

reesscot commented Jan 5, 2022 •
edited
Loading

Hi @carlton-bennett,
The specificity of the border-box rule is 0 1 0, so you should be able to override it with a class:

// index.css
.my-section {
  box-sizing: content-box;
}
 <AmplifyProvider>
            <section
                className="my-section"
                style={!force ? undefined : {
                    boxSizing: 'content-box'
                }}
            />

            <button onClick={() => setForce(!force)}>
                toggle
            </button>
</AmplifyProvider>

Would you please you try this and let us know if it resolves your issue? We depend on this rule for elements we use within our primitives that need box-sizing: border-box. In general I would also say that border-box is a bit more intuitive than the default content-box, so I think it's a sane default to have so long as it can be overridden like the above example.

@carlton-bennett
Copy link
Author

Hi @reesscot, thank you for the response. The example I included is a contrived example to illustrate the issue and doesn't fully reflect our use case. I opted for the simpler example in an effort to keep the issue succinct. Apologies for not including more details about our specific case in the original post.

A class can indeed override this rule but only if it is included after the amplify-ui style sheet. Normally this would be an acceptable solution, but wouldn't be possible for our specific use case. We are using material-ui components combined with regular CSS. The styling for material-ui components are injected using CSS-in-JS and all of the rules are a specificity of 0 1 0. In order to use regular CSS to override these styles with a single class, we need to configure material-ui to inject the CSS-in-JS into the top of the <head>.

This means that the styles for the material-ui components are always defined before the inclusion of the amplify-ui style sheet, so the border-box rule from amplify-ui always takes precedence.

Here's a more precise representation of the issue we are seeing. The TextField component sets border-box: content-box on the input element contained within it, which is overridden by the rule from amplify-ui, causing the text field's height to be incorrect:

// index.tsx
import { AmplifyProvider } from '@aws-amplify/ui-react';
import { StyledEngineProvider, TextField } from '@material-ui/core';
import React  from 'react';
import ReactDOM from 'react-dom';

import '@aws-amplify/ui-react/dist/index.css';


function App() {
    return (
        <StyledEngineProvider injectFirst>
            <div
                style={{
                    display: 'flex',
                    gap: 16
                }}
            >
                <AmplifyProvider>
                    <TextField
                        label="Inside amplify"
                        variant="filled"
                    />
                </AmplifyProvider>

                <TextField
                    label="Outside amplify"
                    variant="filled"
                />
            </div>
        </StyledEngineProvider>
    );
}

ReactDOM.render(
    <App />,
    document.getElementById('root')
);

image

Since we cannot modify the specificity of the rules generated by material-ui nor the source order, the only workaround I have been able to come up with is to define an override for the specific element within the component with a higher specificity, which is a maintenance nightmare.

[data-amplify-theme] .MuiInputBase-input {
  box-sizing: content-box;
}

Do all the primitives that rely on this rule also include a class name that starts with amplify? If so, could you modify this rule to include the border-box declaration? That would maintain the specificity of 0 1 0 without affecting non-amplify elements.

[class*='amplify'] {
  all: unset; /* protect against external styles */
  box-sizing: border-box;
}

Another option would just be to drop the [data-amplify-theme] bit from the selector and just make it a *, which would drop the specificity down to 0 0 0. The only difference with this approach is that it would no longer be scoped to content within the AmplifyProvider, but that's at the root of the app anyway so odds are that this would be functionally equivalent for most use cases.

@reesscot reesscot self-assigned this Jan 13, 2022
@reesscot reesscot mentioned this issue Jan 14, 2022
Merged
@reesscot
Copy link
Contributor

reesscot commented Jan 14, 2022 •
edited
Loading

@carlton-bennett Thanks for the detailed explanation. I can't get your example to run as I'm not sure what version of MUI you are using. Is it possible to post your example to a repo where I can test it? Alternatively, can you please test if the proposed changes in this draft PR will fix your issue?

@reesscot reesscot added pending-response and removed pending-triage Issue is pending triage labels Jan 14, 2022
@carlton-bennett
Copy link
Author

@reesscot I tested the changes proposed in the draft you linked and it does indeed fix the issue. Thank you!!

@reesscot
Copy link
Contributor

reesscot commented Feb 2, 2022

@carlton-bennett The fix has been merged and will be in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@reesscot reesscot

Labels
React An issue or a feature-request for React platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: change box-sizing to have less specificity aws-amplify/amplify-ui
3 participants
@reesscot @Milan-Shah @carlton-bennett