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

chore(Switch): convert Switch to TypeScript #2311

Merged
merged 4 commits into from Jul 2, 2019
Merged

Conversation

@tlabaj
Copy link
Contributor

tlabaj commented Jun 20, 2019

#2019

What:

Additional issues:

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 20, 2019

PatternFly-React preview: https://patternfly-react-pr-2311.surge.sh

/** A callback for when the Switch selection changes. (isChecked, event) => {} */
onChange: PropTypes.func,
onChange?(checked: boolean, event: FormEvent<HTMLInputElement>): void;

This comment has been minimized.

Copy link
@redallen

redallen Jun 24, 2019

Contributor

Use an arrow function so the docs pick this up properly. Like `onChange?: (checked: boolean, event: FormEvent) => void;

'aria-label': ''
};
export class Switch extends React.Component<SwitchProps> {
id = '';

This comment has been minimized.

Copy link
@jessiehuff

jessiehuff Jun 24, 2019

Contributor

Remove this class attribute

This comment has been minimized.

Copy link
@dlabaj

dlabaj Jun 25, 2019

Contributor

Not sure what the purpose of this id is ... is it suppose to be the same as props.id?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 2, 2019

Author Contributor

it instantiates the id when it is generated. And there is no id prop set.

This comment has been minimized.

Copy link
@redallen

redallen Jul 2, 2019

Contributor

Shouldn't it instantiate it as a defaultProp to getUniqueId()?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 2, 2019

Author Contributor

I am not sure. Does it work differently in TS than JS. In JS if you set defaultProp to getUniqueId, if you had more hat one Switch on the page the id's would all be the same? That is why we had it in the constructor before.

This comment has been minimized.

Copy link
@dlabaj

dlabaj Jul 2, 2019

Contributor

Looks like it will set it for all of the name to be the same based off the time. Leave it in the constructor.

@tlabaj tlabaj force-pushed the tlabaj:switch branch from 3c4b8c9 to f168d11 Jun 25, 2019
Copy link
Contributor

dlabaj left a comment

Wondering what was the use case for that separate class attribute ID.

'aria-label': ''
};
export class Switch extends React.Component<SwitchProps> {
id = '';

This comment has been minimized.

Copy link
@dlabaj

dlabaj Jun 25, 2019

Contributor

Not sure what the purpose of this id is ... is it suppose to be the same as props.id?

@tlabaj tlabaj force-pushed the tlabaj:switch branch from 5a0b3b5 to f5c0169 Jun 28, 2019
@tlabaj tlabaj force-pushed the tlabaj:switch branch from f5c0169 to 25d1987 Jul 2, 2019
@dlabaj
dlabaj approved these changes Jul 2, 2019
'aria-label': ''
};
export class Switch extends React.Component<SwitchProps> {
id = '';

This comment has been minimized.

Copy link
@dlabaj

dlabaj Jul 2, 2019

Contributor

Looks like it will set it for all of the name to be the same based off the time. Leave it in the constructor.

@redallen redallen merged commit dac87ba into patternfly:master Jul 2, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.