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

[breakpoints] add back deleted getWidth as width with a spec #8387

Merged
merged 2 commits into from Sep 25, 2017

Conversation

rosskevin
Copy link
Member

@oliviertassinari deleted getWidth probably thinking it is unused. Well it is used and well loved by me ;).

Also updated the typescript, which was still expecting the old named fn.

@oliviertassinari
Copy link
Member

What's the advantage of using it over values[key]?

@rosskevin
Copy link
Member Author

LOL, primarily I had no idea it existed.

I think it's worth exposing it in the interface because otherwise you are making the user dig into the code to know that the values is actually a key map of width. This is true, but if it wasn't obvious to me as a maintainer, would it be apparent to one of our users?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2017

@rosskevin I have been documenting the breaking change in the CHANGELOG.
Maybe this is a documentation issue?

@oliviertassinari
Copy link
Member

I'm worried about extending the API where there is alternative, espically, when users can directly provides their own values property.

@rosskevin
Copy link
Member Author

If they provide an incompatible values property, then they will also have to update the implementation of these methods.

So in the case of extension, it is better to expose an interface than an implementation detail.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2017

@rosskevin But then, why not abstracting the following?

const theme = createMuiTheme({
  palette: {
    secondary: green,
  },
});

color: theme.palette.secondary.A200,

How is that more intuitive than?

const theme = createMuiTheme({
  breakpoints: {
    values: {
      sm: 788,
    }
  },
});


width: theme.breakpoints.values.sm,

@rosskevin
Copy link
Member Author

I don't think your last comment is congruent with your previous postulate that user can provide their own values and that it somehow makes #width a risk.

Yes, accessing values seems quite reasonable, but I could argue that you should never expose values.

It is convenient, and obvious to have #width, that is all, and I do not agree that it is risky.

@oliviertassinari
Copy link
Member

What do you mean by an invalid value property? type Breakpoint = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; is not something they can change. They can only change the breakpoint width value.

@oliviertassinari oliviertassinari merged commit c5d6e86 into mui:v1-beta Sep 25, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2017

@rosskevin Alright, at least we fixed the flow definitions :)

@rosskevin rosskevin deleted the breakpoints-width branch September 27, 2017 22:15
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

Successfully merging this pull request may close these issues.

None yet

2 participants