Skip to content

check muiTheme is defined#1935

Closed
tyfoo wants to merge 3 commits intomui:masterfrom
tyfoo:hotfix/styles-ensureDirection-null-muiTheme
Closed

check muiTheme is defined#1935
tyfoo wants to merge 3 commits intomui:masterfrom
tyfoo:hotfix/styles-ensureDirection-null-muiTheme

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 Higher level theming customizability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants