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

check muiTheme is defined #1935

Closed

Conversation

tyfoo
Copy link
Contributor

@tyfoo tyfoo commented Oct 20, 2015

TimePicker dialog wont open when muiTheme is null/undefined. Steps to reproduce:

  1. Browse to http://material-ui.com/#/components/time-picker
  2. Open the console on browser
  3. Click on any of the TimePicker input fields.
  • Uncaught TypeError: Cannot read property 'isRtl' of undefined

Source: /src/time-picker/clock.jsx line 118 <div style={this.prepareStyles(styles.root)}>, styles.root is {}.

Fix: added a double check to ensureDirection to check muiTheme is not null or undefined before checking its isRtl property.

@oliviertassinari
Copy link
Member

@tyfoo Thanks for this PR. However, the issue you discribed is already fixed by #1922.
@shaurya947 Should we close this one?

@tyfoo
Copy link
Contributor Author

tyfoo commented Oct 20, 2015

@oliviertassinari thanks for the quick feedback.

I am not sure #1922 solves this issue, as clock.jsx does not use muiTheme from the context.

I believe the issue is cause by line 96 in clock.jsx:

let styles = {
      root: {},

      container: {
        height: 280,
        padding: 10,
      },
    };

which it then passes to prepareStyles in line 118:

<div style={this.prepareStyles(styles.root)}>

I believe that gets passed down as undefined or null which throws and exception in ensureDirection.

@shaurya947
Copy link
Contributor

@oliviertassinari I'm thinking if we can solve the problem at its root (prepareStyles)

@shaurya947
Copy link
Contributor

See my last comment on #1674 guys

@tyfoo
Copy link
Contributor Author

tyfoo commented Oct 26, 2015

@oliviertassinari @shaurya947 Thanks for the feedback. Should I close this PR?

@oliviertassinari
Copy link
Member

@tyfoo Yes, I think that we can close this PR since the orignal issue is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants