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

feat(Select): convert to Typescript #2201

Merged
merged 11 commits into from Jul 1, 2019
Merged

feat(Select): convert to Typescript #2201

merged 11 commits into from Jul 1, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Jun 10, 2019

What: Typescript conversion for select. This also contains a couple breaking changes for the Checkbox select variant, whose classes have been collapsed into the normal Select classes (now controlled by the variant parameter only), and CheckboxSelectGroup, which has been renamed SelectGroup and is now generic/available to use for other variants.

Refer to issue: #2096, #2223

2223 should be resolved with the change to TS.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 10, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@a5d571e). Click here to learn what that means.
The diff coverage is 71.53%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2201   +/-   ##
=========================================
  Coverage          ?   80.59%           
=========================================
  Files             ?      664           
  Lines             ?     8610           
  Branches          ?      814           
=========================================
  Hits              ?     6939           
  Misses            ?     1287           
  Partials          ?      384
Flag Coverage Δ
#patternfly3 85.23% <ø> (?)
#patternfly4 76.32% <71.53%> (?)
#patternflymisc 95.79% <ø> (?)
Impacted Files Coverage Δ
...act-core/src/components/Select/selectConstants.tsx 100% <100%> (ø)
.../react-core/src/components/Select/SelectToggle.tsx 61% <61%> (ø)
.../react-core/src/components/Select/SelectOption.tsx 69.49% <69.49%> (ø)
...4/react-core/src/components/Select/SelectGroup.tsx 70% <70%> (ø)
...nfly-4/react-core/src/components/Select/Select.tsx 70.73% <75%> (ø)
...-4/react-core/src/components/Select/SelectMenu.tsx 86.53% <86.53%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5d571e...4cd0d9b. Read the comment docs.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Jun 10, 2019

@redallen Any advice on the failing circleci build? I'm not finding where it's complaining about SelectGroup in the docs.

@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Jun 11, 2019

@kmcfaul Double-check that SelectGroup is correctly exported in the dist. It should be possible to reproduce locally if you start with git clean -dfx.

Copy link
Contributor

redallen left a comment

This is looking awesome, thank you!!!

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 14, 2019

@kmcfaul kmcfaul force-pushed the kmcfaul:ts-select branch from 455f956 to 530e8cb Jun 14, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 14, 2019

@SpyTec

This comment has been minimized.

Copy link
Contributor

SpyTec commented Jun 17, 2019

The failing build in CircleCI seems unrelated as it points to PatternFly 3. But this looks great!

This TypeScript migration doesn't address any existing bugs right? It only migrates existing code and drops some components in favour of one. Is there an ETA for when this is merged?

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 17, 2019

@tlabaj tlabaj added the Do Not Merge label Jun 17, 2019
Copy link
Contributor

tlabaj left a comment

@kmcfaul We are at a point where we are taking in breaking changes. Is there any way to convert the component without introducing any breaking changes?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Jun 18, 2019

Yeah I'll work on a non-breaking change version.

@kmcfaul kmcfaul force-pushed the kmcfaul:ts-select branch from 7bc811f to 5874131 Jun 19, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 19, 2019

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

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Jun 20, 2019

@tlabaj have a patch up that should make this a non-breaking change. The old Checkbox specific classes are back, and users should be able to use them, or use the new way with only Select specific classes, to render the checkbox select. The examples remain updated to show the new method, to further phase out the old method. I have also reverted the prop name change from 'ariaLabel' back to 'aria-label'.

let index = 0;
return React.Children.map(children, (group: React.ReactElement) =>
React.cloneElement(group, {
titleId: group.props.label.replace(/\W/g, '-'),

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Jun 21, 2019

Member

are we certain that the group will always have the label prop?

This comment has been minimized.

Copy link
@kmcfaul

kmcfaul Jun 24, 2019

Author Contributor

label in the Group has a default value so it shouldn't error out

isDisabled?: boolean;
/** Flag indicating if the option acts as a placeholder */
isPlaceholder?: boolean;
/** Internal flag indicating if the option is selected */

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 Jun 21, 2019

Member

What does Internal flag mean?

This comment has been minimized.

Copy link
@kmcfaul

kmcfaul Jun 24, 2019

Author Contributor

It means the property is used by the other select classes, and isn't one that has to be passed in by the user. Our docs generate all props so I wanted to specify which aren't expected.

This comment has been minimized.

Copy link
@redallen

redallen Jun 27, 2019

Contributor

I opened #2380 for this.

Copy link
Contributor

nicolethoen left a comment

Only a couple thoughts and questions. Mostly the same comment multiple times. :)

keyHandler?: (index: number, position: string) => void;
}

export class SelectMenu extends React.Component<SelectMenuProps> {

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Jun 26, 2019

Contributor

I think this can also be a React.FunctionComponent since there is no state.

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Jun 27, 2019

Contributor

and there are no lifecycle methods here, these methods can be defined in the body of the functionComponent

kmcfaul added 2 commits Jun 24, 2019
update tests, minor fixes
@kmcfaul kmcfaul force-pushed the kmcfaul:ts-select branch from 5c803f7 to 7957c69 Jun 27, 2019
Copy link
Contributor

redallen left a comment

I hope it doesn't break too many people.

isDisabled?: boolean;
/** Flag indicating if the option acts as a placeholder */
isPlaceholder?: boolean;
/** Internal flag indicating if the option is selected */

This comment has been minimized.

Copy link
@redallen

redallen Jun 27, 2019

Contributor

I opened #2380 for this.

@tlabaj
tlabaj approved these changes Jul 1, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit fd9fca1 into patternfly:master Jul 1, 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
You can’t perform that action at this time.