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

[docs] Add ExpansionPanels TypeScript Demo #15162

Merged
merged 32 commits into from
Apr 4, 2019
Merged

[docs] Add ExpansionPanels TypeScript Demo #15162

merged 32 commits into from
Apr 4, 2019

Conversation

Adherentman
Copy link
Contributor

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 2, 2019

No bundle size changes comparing 240dc18...b254a2c

Generated by 🚫 dangerJS against b254a2c

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation typescript labels Apr 2, 2019
@@ -67,7 +67,7 @@ function DetailedExpansionPanel(props) {
<ExpansionPanelDetails className={classes.details}>
<div className={classes.column} />
<div className={classes.column}>
<Chip label="Barbados" className={classes.chip} onDelete={() => {}} />
<Chip label="Barbados" onDelete={() => {}} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@@ -106,7 +108,7 @@ class RouterBreadcrumbs extends React.Component {
<List component="nav">
<ListItemLink to="/inbox" open={this.state.open} onClick={this.handleClick} />
<Collapse in={this.state.open} timeout="auto" unmountOnExit>
<List component="div" disablePadding>
<List component={'div'} disablePadding>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something we could improve in the TypeScript to JavaScript logic generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to change the js file directly, but I can't pass this ci. what should i do?
WX20190402-234503@2x

Copy link
Member

@oliviertassinari oliviertassinari Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a topic for a different pull request. I was only raising the potential.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's fine with prettier it's fine with me

@Adherentman
Copy link
Contributor Author

@oliviertassinari

  const [expanded, setExpanded] = React.useState<string | null>(null);

I use u suggest, but i got type and lint error:

Argument of type 'string | false' is not assignable to parameter of type 'SetStateAction<string | null>'.
  Type 'false' is not assignable to type 'SetStateAction<string | null>'.ts(2345)
TypeScript@next compile error: 
Argument of type 'string | false' is not assignable to parameter of type 'SetStateAction<string | null>'.
  Type 'false' is not assignable to type 'SetStateAction<string | null>'. (expect)tslint(1)

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 2, 2019

So, we are using null and false for no value? We can do better.

@Adherentman
Copy link
Contributor Author

So, we are using null and false for no value? We can do better.

nice, I try

  const [expanded, setExpanded] = React.useState<string | false | null>(null);

no error!

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 2, 2019

@Adherentman That's one approach. I think that we can simplify the implementation here. cc @eps1lon.

We could replace the usage of false with null.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Adherentman Could you rebase next again? Looks like this PR was based on #15139

@Adherentman
Copy link
Contributor Author

@Adherentman Could you rebase next again? Looks like this PR was based on #15139

yes, finished

@eps1lon
Copy link
Member

eps1lon commented Apr 3, 2019

@Adherentman Could you rebase next again? Looks like this PR was based on #15139

yes, finished

Doesn't look like it. Did you fetch the upstream before rebasing? If the rebase was successful there shouldn't be any Breadcrumbs related files appear in Files changed.

@eps1lon eps1lon mentioned this pull request Apr 3, 2019
@Adherentman
Copy link
Contributor Author

@Adherentman Could you rebase next again? Looks like this PR was based on #15139

yes, finished

Doesn't look like it. Did you fetch the upstream before rebasing? If the rebase was successful there shouldn't be any Breadcrumbs related files appear in Files changed.

I created a new pr. Because the previous repo was deleted by me. .

@eps1lon eps1lon self-assigned this Apr 3, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be the github UI then. Local comparison is clean.

@Adherentman
Copy link
Contributor Author

Must be the github UI then. Local comparison is clean.

How can I do?

@eps1lon
Copy link
Member

eps1lon commented Apr 3, 2019

Must be the github UI then. Local comparison is clean.

How can I do?

You don't need to do anything. PR is in a good state.

@Adherentman
Copy link
Contributor Author

Must be the github UI then. Local comparison is clean.

How can I do?

You don't need to do anything. PR is in a good state.

nice, Thank you

@eps1lon eps1lon merged commit 239ceb9 into mui:next Apr 4, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 4, 2019

@Adherentman Thanks for sticking with it. Much appreciated, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants