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

AMP is not working with Material-UI #9041

Closed
dmdb opened this issue Oct 11, 2019 · 13 comments · Fixed by mui/material-ui#20463
Closed

AMP is not working with Material-UI #9041

dmdb opened this issue Oct 11, 2019 · 13 comments · Fixed by mui/material-ui#20463
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@dmdb
Copy link

dmdb commented Oct 11, 2019

Examples bug report

Example name

https://github.com/mui-org/material-ui/tree/master/examples/nextjs

Describe the bug

If you turn on AMP-first on a page, for example on about page:
image
You will get error
image

To Reproduce

  1. Follow instructions https://github.com/mui-org/material-ui/tree/master/examples/nextjs
  2. Enable AMP on page
  3. Open it
  4. See error

Expected behavior

AMP is working fine with Material UI

System information

  • OS: Ubuntu 18.04
  • Chrome
  • Next.js 9.1.1, MaterialUI 4.5.0

Additional context

It maybe problem with material-ui example or other styling integrations. Here is some similar as I think issues:
#7856
mui/material-ui#17784

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers help wanted labels Oct 11, 2019
@Janpot
Copy link
Contributor

Janpot commented Oct 11, 2019

@dmdb FYI, in case you want to pick this up, an effort to fix this was started in #7954, I'm not 100% sure where this should be headed, but I guess the with-typescript-styled-components` example should be updated to use

        styles: [
          ...React.Children.toArray(initialProps.styles),
          ...sheet.getStyleElement(),
        ],

to make it work and compliant with the types. I can only guess that the same kind of setup was used in other CSS-in-JS examples and will have the same incompatibilities with AMP.

@dmdb
Copy link
Author

dmdb commented Oct 11, 2019

@Janpot Unfortunately I'am not quite confident to pick it, I've just monkey-patched it in our project (thanks to @tomevans18) with
image
It seems works fine, but not sure why 🤷‍♀️
Anyway it seems that it is a common problem with example and should be fixed somehow in proper, official way.

PS. Thanks for AMP support on Next.js team. It is a huge benefit

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Oct 29, 2019

We have updated Material-UI's examples to work with Next.js and AMP, and now waiting on mui/material-ui#18044 to solve the TypeScript part of it.

@derolf
Copy link

derolf commented Feb 17, 2020

I am trying to get nextjs with amp and material-ui to work, but the styles don’t get included in amp mode (works in non-amp mode). Is there any minimal example?

@KyruCabading
Copy link

KyruCabading commented Apr 7, 2020

@oliviertassinari saw this and wanted to mention that NextJS has a custom Html component within next/document that might be what's missing for AMP support. One of our devs noticed that new component in a NextJS MUI PR

If you look at docs here, there's a new Html component to use within _document.js
https://nextjs.org/docs/advanced-features/custom-document

And that component adds an amp prop which is required for AMP support.
https://github.com/zeit/next.js/blob/canary/packages/next/pages/_document.tsx#L138-L165

https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/

We can add it in if you'd like

@oliviertassinari
Copy link
Contributor

@KyruCabading As far as I know, we have fixed this issue, it can be closed.

@timneutkens
Copy link
Member

Opened mui/material-ui#20463, good catch @KyruCabading

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Apr 8, 2020

@timneutkens Thanks, we are making progress:

Capture d’écran 2020-04-08 à 11 48 18


However, when I try to render this page

import React from 'react';
import TextField from '@material-ui/core/TextField';

export default function BasicTextFields() {
  return (
    <div>
      <TextField id="standard-basic" label="Standard" />
    </div>
  );
}

export const config = { amp: true }

I get the following error:

Creating an optimized production build

Compiled successfully.

Amp Validation

/  error  The text inside tag 'style amp-custom (transformed)' contains 'CSS !important', which is disallowed.  https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets
   error  The text inside tag 'style amp-custom (transformed)' contains 'CSS !important', which is disallowed.  https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets
   error  The text inside tag 'style amp-custom (transformed)' contains 'CSS !important', which is disallowed.  https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets
   error  The text inside tag 'style amp-custom (transformed)' contains 'CSS !important', which is disallowed.  https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#stylesheets


> Build error occurred
Error: AMP Validation caused the export to fail. https://err.sh/zeit/next.js/amp-export-validation

I'm investigating.

@timneutkens
Copy link
Member

cc @sebastianbenz

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Apr 8, 2020

It works if I comment the !important. I will see if we can work around it in the core. The only concern I have left is that we rely on the React runtime to update the styles on focus. We lose this visual feedback.

Capture d’écran 2020-04-08 à 12 05 29

import React from 'react';
import TextField from '@material-ui/core/TextField';

export default function BasicTextFields() {
  return (
    <div>
        <TextField InputLabelProps={{shrink: true}} id="standard-basic" label="Standard" />
        <TextField InputLabelProps={{shrink: true}} id="filled-basic" label="Filled" variant="filled" />
        <TextField InputLabelProps={{shrink: true}} id="outlined-basic" label="Outlined" variant="outlined" />
    </div>
  );
}

export const config = { amp: true }

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Apr 8, 2020

We use !important to fix an issue with Edge. We have an alternative solution at mui/material-ui#15183 (comment) but I'm not sure it's worth fixing. I propose we wait to see users' requests for it.

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants