-
Notifications
You must be signed in to change notification settings - Fork 42
Ekspanderbartpanel typescript #233
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
Conversation
@@ -0,0 +1,5 @@ | |||
/// <reference types="react" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fint hvis du fjerner alle disse definisjonsfilene. De blir generert automatisk ved bygg, så de trenger vi ikke :) Forøvrig litt rart at de ligger under src
hos deg. Bruker du npm run build
i rot-katalogen for å bygge .tsx
-filene?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Har brukt npm run build
ja, før jeg ble fortalt om npm run gulp build
. start-guideline-app
fungerte dårlig for meg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hva var det som ikke fungerte med start-guideline-app
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Et par ting :)
heading: React.ReactNode; | ||
className?: string; | ||
onClick: (event: React.SyntheticEvent<HTMLButtonElement>) => void; | ||
ariaTittel?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, skal denne være optional?
import 'nav-frontend-ekspanderbartpanel-style'; // eslint-disable-line import/extensions | ||
import EkspanderbartpanelBasePure from './ekspanderbartpanel-base-pure'; | ||
|
||
class EkspanderbartpanelBase extends Component { | ||
export interface EkspanderbartpanelBaseProps { | ||
apen: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skal denne være required?
@@ -2,8 +2,8 @@ | |||
"name": "nav-frontend-ekspanderbartpanel", | |||
"version": "0.2.21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump major
constructor(props) { | ||
super(props); | ||
|
||
this.state = { | ||
apen: this.props.apen | ||
apen: this.props.apen ? this.props.apen : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kanskje ha en default verdi på denne i stedet for å sjekke om den finnes?
class EkspanderbartpanelBase extends React.Component<EkspanderbartpanelBaseProps, EkspanderbartpanelBaseState> {
static defaultProps: Partial<EkspanderbartpanelBaseProps> = {
apen: false
}
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
God idé, men rett ut av boksen gir det følgende feil:
packages/node_modules/nav-frontend-ekspanderbartpanel/src/index.tsx(35,9): error TS2322: Type '{ apen: boolean | undefined; }' is not assignable to type 'Readonly<EkspanderbartpanelState>'.
Types of property 'apen' are incompatible.
Type 'boolean | undefined' is not assignable to type 'boolean'.
Type 'undefined' is not assignable to type 'boolean'.
Må lure ts-kompilatoren her for å få dette til å fungere, hvis ikke ts selv har noe krydder vi kan spe på med :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, du kan undertrykke den feilmeldingen ved å hekte på en !
bak variabelen for å fortelle kompilatoren at den aldri er null
/undefined
, ala:
this.state = {
apen: this.props.apen!
};
Ref. dokumentasjon. Se tredje avsnitt under "Type guards and type assertions" ca. halvveis ned på siden.
export interface EkspanderbartpanelPureProps { | ||
onClick: (event: React.SyntheticEvent<HTMLButtonElement>) => void; | ||
tittel: string; | ||
tittelProps: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kanskje beholde denne som optional med default verdi = undertittel
?
@@ -16,16 +16,17 @@ | |||
"classnames": "^2.2.5", | |||
"nav-frontend-ekspanderbartpanel-style": "^0.3.8", | |||
"nav-frontend-typografi": "^2.0.2", | |||
"prop-types": "^15.5.10", | |||
"react": "^15.4.2 || ^16.0.0", | |||
"react": "^16.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skal ikke denne komponenten fungere med react@15 noe mer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Den kan jo det, men er det ikke greit å enforce react 16 når vi likevel bumper major?
Eller blir det bråk av slik tankegang? :P
Det er dratt inn fra nav-frontend-moduler. Per nå så opplever jeg litt krøll med komponenten, sannsynligvis fordi den ikke er skrevet i React. InteliJ rapporterer at 'EkspanderbartpanelBase does not have any construct or call signature'. Det funker fint å kompilere og kjøre, men skriptet feiler ved hot-reload av visittkort-komponenten. Det kan være relatert til denne: microsoft/TypeScript#14558. Denne har blitt merget inn i master, men er ikke ute i en release enda. Det eksisterer også en PR i nav-frontend på å skrive om panelet til typecript som forhåpentligvis vil løse problemet: navikt/aksel#233
# Conflicts: # packages/node_modules/nav-frontend-ekspanderbartpanel/package.json
Takk for hjelpen @Lillebo! :) |
No prob, takk selv :) |
No description provided.