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

[autoprefixer] Fix a style issue with user agent all and display flex #5668

Merged
merged 1 commit into from
Nov 27, 2016
Merged

[autoprefixer] Fix a style issue with user agent all and display flex #5668

merged 1 commit into from
Nov 27, 2016

Conversation

oliviertassinari
Copy link
Member

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

This is a continuation of #5648.
Thanks @masakij for raising this issue

Fix #5648.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Nov 27, 2016
@oliviertassinari oliviertassinari changed the title [autoprefixer] Fix a style issue with user agent all and display flex" [autoprefixer] Fix a style issue with user agent all and display flex Nov 27, 2016
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Nov 27, 2016
This is a continuation of #5648.
Thanks @masakij for raising this issue

Fix #5648.
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Nov 27, 2016
@oliviertassinari oliviertassinari merged commit 2b57f81 into mui:master Nov 27, 2016
@oliviertassinari oliviertassinari deleted the style-ua-all-flex branch November 27, 2016 19:44
@masakij
Copy link

masakij commented Nov 29, 2016

Thanks @oliviertassinari for accepting my issue. And sorry for late reply.

But My app didn't work with this patch (like 👇) because of UA: 'all' universal configuration for SSR caching...
image

hmm...

@oliviertassinari
Copy link
Member Author

@masakij Could you provide more details regarding this issue?

@masakij
Copy link

masakij commented Nov 30, 2016

My app has an universal ThemeProvider like below.

const muiTheme = getMuiTheme(baseTheme, { userAgent: 'all' });
class App extends React.Component {
  render() {
    return (
      <MuiThemeProvider muiTheme={muiTheme}>
        {children}
      </MuiThemeProvider>
    );
  }
}

This provider is used by server & client.
SSR works good, but client side didn't work. For example, Avatar is rendered below.

<Avatar>A</Avatar>

⬇️

<div size="40"
  style="
    color:#ffffff;background-color:rgb(188, 188, 188);
    user-select:none;
    display:-webkit-box,-moz-box,-ms-inline-flexbox,-webkit-inline-flex,inline-flex;
    align-items:center;
    justify-content:center;
    font-size:20px;
    border-radius:50%;
    height:40px;
    width:40px;
    mui-prepared:;
    -webkit-user-select:none;
    -moz-user-select:none;
    -ms-user-select:none;
    -webkit-align-items:center;
    -webkit-justify-content:center;
    -ms-flex-align:center;
    -webkit-box-align:center;
    -ms-flex-pack:center;
    -webkit-box-pack:center;
  "
  data-reactid="7">
    <!-- react-text: 8 -->A<!-- /react-text -->
</div>

Rendered element's display value is broken.

I tried another way specifing userAgent. It works, but get invalid checksum warning.
I think that it makes SSR worthless, so I'm looking for another solution like CSS now.

Thanks

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 30, 2016

but client side didn't work

That's expected, I have linked this issue facebook/react#6467 in the first PR. You can't prefix for all user-agent on the client.
I don't think that we should aim for this. It's making the style injection much slower. We already know the user agent when rendering on client side. So, what's the point?

The server-side rendering with user-agent: all is another topic.
It depends on the cache size tradeoff you want to make.

but get invalid checksum warning.

IMHO, that's totally fine. There is an issue on the React repository regarding that point.

so I'm looking for another solution like CSS now.

You can have a look at the next branch. We have addressed that issue.

@masakij
Copy link

masakij commented Dec 1, 2016

That's expected,

I tested just in case

It's making the style injection much slower.

hmm... It's not good. I'd like to make sure that SSR is not ignored.
I'll search for UA specific caching.

You can have a look at the next branch. We have addressed that issue.

It's sounds good. I'll check it.
When will you release it?

@idangozlan
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants