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

<MatrialUi.WithTheme> crash if classes property only is provided #17

Closed
unodgs opened this issue Jul 28, 2018 · 9 comments
Closed

<MatrialUi.WithTheme> crash if classes property only is provided #17

unodgs opened this issue Jul 28, 2018 · 9 comments

Comments

@unodgs
Copy link

unodgs commented Jul 28, 2018

This code worked in previous versions:

render: self => <MaterialUi.WithStyles
        classes=[{
            name: "fullScreen",
            styles: Style.make(~width="100vw", ~height="100vh", ())
        }, {
            name: "fullView",
            styles: Style.make(~width="100%", ~height="100%", ())
        }, {

but recently it stopped with following error:
image

If I add classesWithTheme={theme => []} then my code runs correctly again. But it shouldn't be necessary IMO.

@unodgs unodgs changed the title <MatrialUi.WithTheme> crash if classes property only is provided <MatrialUi.WithTheme> crash if classes property only is provided Jul 28, 2018
@jsiebern
Copy link
Collaborator

You're absolutely right, that should work fine! I'll take a look!

@jsiebern
Copy link
Collaborator

Actually, that might be due to a regression in bsb that has been fixed since but not published to my knowledge! I'll have to test that.

@PTaylour
Copy link

PTaylour commented Sep 5, 2018

Think of a way to use theme => object pattern as withStyles argument(2017-11-25)

Is this comment in the readme referring to the classesWithTheme prop?

If so, is this the only way to style with access to the theme, or is there also plans for it to be accessible via a ppx?

If there is anything I can do to help, give me a shout :)

@jsiebern
Copy link
Collaborator

jsiebern commented Sep 8, 2018

@PTaylour Thanks for the offer, I'm overwhelmed at work right now, so if you want to take a look at the ppx / theme integration (the object is not fully typed and yet and bs.abstract didn't even exist when I did that) I'd be happy to accept pull requests :)

@PTaylour
Copy link

I'll take a look :). Still new to all of this (and my bucklescript level of success has so far been mixed) — but no harm giving it a go!

@PTaylour
Copy link

@jsiebern So a good thing for me to start with would be to type the rest of the theme object here https://github.com/jsiebern/reason-mui-binding-generator/blob/master/src/fixed-modules/MaterialUi_Theme.re using [@bs.deriving abstract]?

@jsiebern
Copy link
Collaborator

@PTaylour Yes, I believe that would be a good place to start! I believe that bs still has no adequate way of converting / accessing multiple object levels, so that makes it a bit challenging.

Could you open a new issue for this please? I fixed the bug discussed in this issue just now and will close it once published

@jsiebern
Copy link
Collaborator

Could you kindly check @next and let me know if this issue can be closed?

@jsiebern
Copy link
Collaborator

jsiebern commented Nov 2, 2018

Fixed in 0.4.2

@jsiebern jsiebern closed this as completed Nov 2, 2018
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

No branches or pull requests

3 participants