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(Grid): Convert grid to typescript #2443

Merged
merged 13 commits into from Jul 10, 2019
Merged

Conversation

@jessiehuff
Copy link
Contributor

jessiehuff commented Jul 3, 2019

Fixes #2447

redallen added 2 commits Jul 3, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 3, 2019

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

Jessie added 5 commits Jul 5, 2019
Copy link
Contributor

tlabaj left a comment

We are missing demo and integration test here..

@@ -5,7 +5,7 @@ import { Title, TitleSize, TitleLevel } from '.';
Object.values(TitleSize).forEach(size => {
test(`${size} Title`, () => {
const view = shallow(
<Title size={size} headingLevel={TitleLevel.h1}>
<Title size={size as 'xs' | 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'} headingLevel={TitleLevel.h1}>

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 8, 2019

Contributor

does the Title.h1 work here? Or do we have to use the string?

This comment has been minimized.

Copy link
@jessiehuff

jessiehuff Jul 8, 2019

Author Contributor

Yes, it should still work. :)

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 9, 2019

Contributor

Okay, butwhy are we changing this as part of the Grid conversion?

This comment has been minimized.

Copy link
@jessiehuff

jessiehuff Jul 9, 2019

Author Contributor

I was running into some errors with the tests, and this was my workaround at the time. I think I found a way that we can keep the previous code.

md: 'Md',
lg: 'Lg',
xl: 'Xl',
xl2: '_2xl'

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 8, 2019

Contributor

Why did we change the definition of the const here?

This comment has been minimized.

Copy link
@jessiehuff

jessiehuff Jul 8, 2019

Author Contributor

This is to build the prop names from react-styles correctly.

Jessie
@@ -5,7 +5,7 @@ import { Title, TitleSize, TitleLevel } from '.';
Object.values(TitleSize).forEach(size => {
test(`${size} Title`, () => {
const view = shallow(
<Title size={size} headingLevel={TitleLevel.h1}>
<Title size={size as 'xs' | 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl' | '4xl'} headingLevel={TitleLevel.h1}>

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 9, 2019

Contributor

Okay, butwhy are we changing this as part of the Grid conversion?

Jessie added 2 commits Jul 9, 2019
Jessie
Jessie
@tlabaj
tlabaj approved these changes Jul 10, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Contributor

redallen left a comment

Nice refactor.

@redallen redallen merged commit d4f8231 into patternfly:master Jul 10, 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
4 participants
You can’t perform that action at this time.