Semantic-ui-react: 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

• [x] #1216 Button
• [x] #1159 Container
• [x] #1155 Flag
• [x] #1159 Header
• [x] #1159 Icon
• [x] #1155 Image
• [x] #1167 Input
• [x] #1265 Label
• [x] #1270 List
• [x] #1165 Loader
• [x] #1171 Rail
• [x] #1165 Reveal
• [x] #1165 Segment
• [x] #1203 Step
• [x] #1209 Breadcrumb
• [x] #1320 Form
• [x] #1231 Grid
• [x] #1264 Menu
• [x] #1254 Message
• [x] #1200 Table
• [x] #1284 Card
• [x] #1293 Comment
• [x] #1285 Feed
• [x] #1294 Item
• [x] #1232 Statistic
• [x] #1333 Accordion
• [x] #1155 Checkbox
• [x] #1208 Dimmer
• [x] #1334 Dropdown
• [x] #1159 Divider
• [x] #1217 Embed
• [x] #1331 Modal
• [x] #1322 Popup
• [x] #1269 Progress
• [x] #1253 Rating
• [x] #1332 Search
• [x] #1296 Sidebar
• [x] #1282 Confirm
• [x] #1300 Portal
• [x] #1155 Radio
• [x] #1171 Select
• [x] #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!