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

[Tabs][BottomNavigation] Use value over index property #7741

Merged
merged 1 commit into from
Aug 12, 2017
Merged

[Tabs][BottomNavigation] Use value over index property #7741

merged 1 commit into from
Aug 12, 2017

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Aug 12, 2017

This is an effort in the prolongation of #2957. I believe it's the last inconsistency we have in that dimension.

-<Tabs index={0}>
+<Tabs value={0}>
// ...
  • 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).

@oliviertassinari oliviertassinari merged commit a2c64e0 into mui:v1-beta Aug 12, 2017
@oliviertassinari oliviertassinari deleted the normalize-value-api branch August 12, 2017 15:39
sebald pushed a commit to PTW-Freiburg/material-ui that referenced this pull request Aug 15, 2017
* upstream/v1-beta: (24 commits)
  v1.0.0-beta.5
  Update CHANGELOG.md
  [CHANGELOG] Prepare v1.0.0-beta.5
  chore(package): update size-limit to version 0.9.0 (mui#7757)
  [docs] Fix yarn docs:build script (mui#7745)
  [docs] Update the readme
  [Radio] Fix accessibility issue (mui#7742)
  [Tabs][BottomNavigation] Use value over index property (mui#7741)
  [core] Remove createStyleSheet (mui#7740)
  [core] Start simplifying styling API (mui#7730)
  [LinearProgress] Use transform instead width (mui#7732)
  Fix Typo (mui#7736)
  Documentation Fix - Fixing up the README documentation (mui#7733)
  [ButtonBase] Expose internal component (mui#7727)
  [Popover] Refactor popover transition - separation of concerns (mui#7720)
  Button documentation fix (mui#7726)
  Update supported-components.md (mui#7722)
  chore(package): update webpack to version 3.5.3 (mui#7723)
  [flow] flow type transitions Slide, Fade, Collapse (fixes)
  Create CODE_OF_CONDUCT.md
  ...
@ljvanschie
Copy link
Contributor

I don't know if this is the place to mention this, but the transition from index to value has not been implemented in the typings:

https://github.com/callemall/material-ui/blob/v1-beta/typings/index.d.ts#L1352

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 16, 2017

@sebald now you see why I think that automatic typescript generation is important. I have to admit, I haven't done any effort to keep the definitions up to date. But we can't expect external contributors to think about it. They already have to take into account the lint, flow-typed and test coverage.

@sebald
Copy link
Member

sebald commented Aug 16, 2017

@oliviertassinari Yes, that's what I am here for :)

I only can speak of personal experience, having typings of libs that are not written in TypeScript was never great. 1-2yrs it was actually pretty pretty bad. I still run into issues with typings on a weekly basis and have lots of custom typings or typings that overwrite bad/old typings from @types/*.

All I can say is that I "build" the typings in a way so that if anyone is running into troubles, they can easily overwrite certain typings. This is sadly not always they case.

Because I now know that you'll release over the weekend, I reserved myself a time slot at work to update typings on Friday. So my team can also update to the latest beta on Monday/Tuesday :)

If PRs get tagged as BREAKING CHANGE it should be possible to keep the typings up to date. If you think that the current approach doesn't work I can try to setup a POC using tsgen over the course of next week and we'll see if this work better :)

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 16, 2017

@sebald I won't say that the current approach doesn't work as we haven't had the opportunity to try it out yet. I would rather wait a month or two to see how that goes. At the end of the day, if your team is happy with the typescript coverage of the project we are good. The experiences from the rest of the community should be pretty close. This is a good proxy. If you think that tsgen can save us time and energie, go ahead otherwise don't :).

sebald pushed a commit that referenced this pull request Aug 17, 2017
sebald added a commit that referenced this pull request Aug 18, 2017
- Update `<Tabs>` API to match BC introduced in #7741
- Update `withStyle` API to match BC introduced in #7740
- Remove discriminated unions (microsoft/TypeScript#17882)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants