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

standard css properties for react.d.ts #5089

Closed
wants to merge 3 commits into from

Conversation

stepancar
Copy link
Contributor

i tryed to add style attribute: style={{display: 'block'}}> but CSSProperties not contains 'display'.
As i understand, should populate this interface with snadard css properties?

i tryed to add style attribute: style={{display: 'block'}}> but CSSProperties not contains 'display'.
As i understand, should populate this interface with snadard css properties?
@jbrantly
Copy link
Contributor

This seems reasonable to me. However, changes should be made to both react.d.ts and react-global.d.ts.

@vsiao Do you know why CSSProperties is incomplete? Is there any reason we can't try to make it complete?

@vsiao
Copy link
Contributor

vsiao commented Jul 27, 2015

I didn't populate CSSProperties fully because... well, there's a lot of them. It doesn't have to be a complete definition to work (in the context of creating DOMElements), since all of these properties are optional anyway. @stepancar, can you explain why it is helpful for you to have the display property in CSSProperties? Are you getting any compilation errors?

@jbrantly
Copy link
Contributor

I wonder if @stepancar is using the nightly build (TS 1.6) which has this. That could cause an error in this case,. With that in place, you would have to do style={<any>{display: 'block'}} I believe.

@stepancar
Copy link
Contributor Author

@vsiao, for example: We have

interface State{
     isClosed: boolean;
}

and in render function

render(){
     return <div style={{display: this.state.isClosed? 'none': 'block'}}></div>
}

you'll catch typeError, beacause display isn' exists in CSSPropertis Type which has display attribute.
I know, this is nor best approach to hide/display block in React - you can just add if-statment;
But this approach has a right to exist (i, sorry for my english)
I think, also max-height, min-height ... shoulde be contained in CSSProperties. I faced some cases when could not do without javascript which calculate max-height.
If you try add max-height style - you'll catch typeError lilke with display.
@jbrantly style={<any>{display: 'block'}} - looks little derty, and it's not work (i checked it using atom-typescript)

@jbrantly
Copy link
Contributor

@stepancar Apologies, I just realized you're using JSX which doesn't have support for angle-bracket type assertion. Try style={{display: 'block'} as any}.

@stepancar
Copy link
Contributor Author

@jbrantly thank you! It works.
but i think that we should still populate CSSProperties with display,max-height, min-height for best autocomplete and cssprop checking at compile stage.

@jbrantly
Copy link
Contributor

I agree. A large point of that fix is to be able to catch typos and such (ex: style={{dislpay: 'block'}}). That obviously won't work unless we have largely complete definitions for style properties.

@vvakame
Copy link
Member

vvakame commented Jul 28, 2015

well... can I merge this PR? @pspeter3 @vsiao

@jbrantly
Copy link
Contributor

It should at the very least have the same changes applied across the board before a merge. react.d.ts, react-addons.d.ts, and react-global.d.ts.

@pspeter3
Copy link
Contributor

pspeter3 commented Aug 4, 2015

It looks fine to merge pending changes to all files.

@stepancar
Copy link
Contributor Author

@jbrantly, hello! I added additional props to react.d.ts, react-global.d.ts
i think it might be merged

@robertknight
Copy link
Contributor

I've run into the issue @jbrantly just described with TS 1.6 where keys in object literals must match identifiers listed in interface properties.

Given the large number of possible properties, not accounting for vendor-specific properties, an option would be to add a generic index property to CSSProperties as a fallback. This would enable catching of incorrect types passed to numeric properties, but wouldn't catch typos in property names:

interface CSSProperties {
    flex: number;
    display: string;

    [index: string]: string | number;
}

An alternative would be to list all the standard properties from CSS 3 in CSSProperties and require users to create their own subclassing interfaces for additional properties. Thoughts?

@jbrantly
Copy link
Contributor

@robertknight I think the right approach is to list all of the properties so that we get the benefits of TypeScript and the new freshness checking for catching typos. Otherwise we're throwing that benefit away. It's been on my list of things to-do for a while now though so... it's just a matter of someone getting the bandwidth to actually make the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants