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 Select TypeScript demos #15288

Merged
merged 12 commits into from
Apr 12, 2019
Merged

[docs] Add Select TypeScript demos #15288

merged 12 commits into from
Apr 12, 2019

Conversation

cahilfoley
Copy link
Contributor

Added TypeScript versions of all Select demos for #14897.

As a possible enhancement for the future - once the TypeScript version in material-ui is upgraded to ^3.4.0 (for the const assertions feature) an improvement can be made to the Name type in MultipleSelects.tsx by declaring the names variable as a readonly array.

const names = [
  'Oliver Hansen',
  'Van Henry',
  'April Tucker',
  'Ralph Hubbard',
  'Omar Alexander',
  'Carlos Abbott',
  'Miriam Wagner',
  'Bradley Wilkerson',
  'Virginia Andrews',
  'Kelly Snyder',
-];
+] as const;

-type ValuesOf<T extends string[]> = T[number];
+type ValuesOf<T extends readonly string[]> = T[number];
type Name = ValuesOf<typeof names>;

This prevents the type widening on the Name type which makes the signature awesome

-type Name = string
+type Name = "Oliver Hansen" | "Van Henry" | "April Tucker" | "Ralph Hubbard" | "Omar Alexander" | "Carlos Abbott" | "Miriam Wagner" | "Bradley Wilkerson" | "Virginia Andrews" | "Kelly Snyder"

I've setup the rest of the demo to take advantage of this when it becomes available 😀

@eps1lon eps1lon mentioned this pull request Apr 9, 2019
@eps1lon eps1lon self-requested a review April 9, 2019 15:40
@eps1lon eps1lon added docs Improvements or additions to the documentation typescript labels Apr 9, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 9, 2019

No bundle size changes comparing 26fcc0c...44c7d51

Generated by 🚫 dangerJS against 44c7d51

age: '',
});

const handleChange = (name: keyof typeof state) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a preference, but the state type could be broken into its own interface, referenced in the useState call above, and then used here as well!

Copy link
Contributor Author

@cahilfoley cahilfoley Apr 10, 2019

Choose a reason for hiding this comment

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

That's totally fair! I usually tend to use type inference when it will be handled correctly, especially for state hooks as there could be quite a few and it can get a bit tedious to write an interface for each if they are simple; that's just a preference too though - if you think it would be better to explicitly declare the interfaces in the examples then I'm happy to change it 👌

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see! That makes sense to me!

docs/src/pages/demos/selects/MultipleSelect.tsx Outdated Show resolved Hide resolved
docs/src/pages/demos/selects/MultipleSelect.tsx Outdated Show resolved Hide resolved
setLabelWidth(inputLabel.current ? inputLabel.current.offsetWidth : 0);
}, []);

const handleChange = (name: keyof typeof state) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied to the above

@eps1lon
Copy link
Member

eps1lon commented Apr 10, 2019

@cahilfoley Could you rebase onto next please? We made some changes to value types across multiple components.

@eps1lon eps1lon added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 10, 2019
@cahilfoley
Copy link
Contributor Author

Sure, I'll do it as soon as I get home from work (~10hrs)

const inputLabel = React.useRef<HTMLLabelElement>(null);
const [labelWidth, setLabelWidth] = React.useState(0);
React.useEffect(() => {
setLabelWidth(inputLabel.current!.offsetWidth);
Copy link
Member

Choose a reason for hiding this comment

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

We can be pragmatic here I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it would be better to have it split into an interface?

Copy link
Member

Choose a reason for hiding this comment

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

I meant the non-null assertion. There is a subtle issue with this approach but that's not related to typescript.

@eps1lon eps1lon removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 12, 2019
@eps1lon eps1lon merged commit dea2617 into mui:next Apr 12, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 12, 2019

@cahilfoley Great job. Thanks!

@cahilfoley cahilfoley deleted the docs/select-demos-in-ts branch April 13, 2019 04:46
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