fix(typings): unable to set many typical html element props

[dennari]

I was running in to problems trying to set the id attribute for Segment. Currently the typings don't allow this in TSX, even if it would be perfectly ok in JSX. The same is true for a number of other standard html attributes and Semantic-UI-React-Components.

One option would be to change

interface SegmentProps {

to

interface SegmentProps extends React.HTMLProps<HTMLElement> {

giving the ability to set a huge amount of additional attributes, including id. This however floods the intellisense with attributes that are probably unneeded 95% of the time.

Another option would be to add an index type:

interface SegmentProps {
  ...
  [key: string]: any;
}

which would then allow any property to be set without flooding intellisense.

Any thoughts?

[levithomason]

I would assume the latter would be more useful to users. That said, I'd like to get some feedback on this from our ts users before merging a fix.

/cc @tomitrescak @hlehmann @jasonritchie @garyham @asvetliakov @halfmatthalfcat @psudarsa

You all have been active in our ts decisions in the past, let us know your thoughts here.

[asvetliakov]

For now you can use module augmentation i think

declare module "semantic-ui-react" {
interface SegmentProps {
   id?: string;
}
// or
interface SegmentProps extends React.HTMLProps<HTMLElement> {}
}

Personally i'm doing 1) approach for elements where passing html attributes does make sense (input, form, etc...)

The 2) approach destroys parameter validation since you can pass anything.

[levithomason]

The 2) approach destroys parameter validation since you can pass anything.

Good point, this seems rather unfavorable.

[dennari]

The 2) approach destroys parameter validation since you can pass anything.

The validation still works as expected for the explicitly specified properties.

[asvetliakov]

Yes, seems i was wrong about 2. Latest compiler version works as expected.

[levithomason]

If there is an agreement on 2, I'd merge a PR for that. It would be nice not to have the common HTML props flooding the completions. I think the intent is for users to see the custom component props.

[levithomason]

For clarity, we're ready for a community PR here implementing this pattern for all components:

interface SegmentProps {
  ...
  [key: string]: any;
}

[layershifter]

TODO

• style(Button): update typings and propTypes usage #1216 Button
• style(Container|Divider|Header|Icon): propTypes cleanups and typings update #1159 Container
• style(Checkbox|Flag|Image|Radio): propTypes cleanups and typings update #1155 Flag
• style(Container|Divider|Header|Icon): propTypes cleanups and typings update #1159 Header
• style(Container|Divider|Header|Icon): propTypes cleanups and typings update #1159 Icon
• style(Checkbox|Flag|Image|Radio): propTypes cleanups and typings update #1155 Image
• style(Input): propTypes cleanups and typings update #1167 Input
• style(Label): update typings and remove propTypes #1265 Label
• style(List): update typings and remove propTypes #1270 List
• style(Loader|Reveal|Segment): propTypes cleanups and typings update #1165 Loader
• style(Rail|Select|TextArea): cleanup typings #1171 Rail
• style(Loader|Reveal|Segment): propTypes cleanups and typings update #1165 Reveal
• style(Loader|Reveal|Segment): propTypes cleanups and typings update #1165 Segment
• style(Step): update typings, tests and propTypes usage #1203 Step
• style(Breadcrumb): update typings and propTypes usage #1209 Breadcrumb
• style(Form): update typings and propTypes usage #1320 Form
• style(Grid): update typings and propTypes usage #1231 Grid
• style(Menu): update typings and remove propTypes #1264 Menu
• style(Message): update typings and remove propTypes #1254 Message
• style(Table|mixed): update typings, tests and propTypes usage #1200 Table
• style(Card): update typings and propTypes usage #1284 Card
• style(Comment): update typings and propTypes usage #1293 Comment
• style(Feed): update typings and propTypes usage #1285 Feed
• style(Item): update typings and propTypes usage #1294 Item
• style(Statistic): update typings and propTypes usage #1232 Statistic
• style(Accordion): update typings #1333 Accordion
• style(Checkbox|Flag|Image|Radio): propTypes cleanups and typings update #1155 Checkbox
• style(Dimmer): update typings #1208 Dimmer
• style(Dropdown): update typings and propTypes usage #1334 Dropdown
• style(Container|Divider|Header|Icon): propTypes cleanups and typings update #1159 Divider
• style(Embed): update typings and propTypes usage #1217 Embed
• style(Modal): update typings and propTypes usage #1331 Modal
• style(Popup): fix tests, update typings and propTypes usage #1322 Popup
• style(Progress): refactor, update typings and remove propTypes #1269 Progress
• style(Rating): update typings and propTypes usage #1253 Rating
• style(Search): update typings #1332 Search
• style(Sidebar): update typings and propTypes usage #1296 Sidebar
• style(Confirm): refactor, update typings and propTypes usage #1282 Confirm
• style(Portal): update typings and propTypes usage #1300 Portal
• style(Checkbox|Flag|Image|Radio): propTypes cleanups and typings update #1155 Radio
• style(Rail|Select|TextArea): cleanup typings #1171 Select
• style(Rail|Select|TextArea): cleanup typings #1171 Text Area

[dstpierre]

@levithomason I'm interested in completing this, can my PR contains all the component listed in the TODO above?

Thanks

[levithomason]

@dstpierre awesome! I'd prefer we do only a few at a time. It is easier to review and manage conflicts that way. Also, anytime there is a list of items todo like this, there always seems to be other small bug fixes and updates that are necessary along the way. Keeping the PRs small keeps the changes manageable, whereas, grouping them tends to lead to a massive PR that has a very hard time getting merged.

[levithomason]

@dstpierre looks like you're in a mad race with @layershifter for this one! 😉

[layershifter]

It's done 😄

[levithomason]

🎉 all praise be to you @layershifter, completely amazing work!