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(LoginPage): Convert LoginPage to typescript, add integration tests, and demo #1967

Merged
merged 3 commits into from May 17, 2019

Conversation

@jeff-phillips-18
Copy link
Member

jeff-phillips-18 commented May 10, 2019

What:

  • Convert LoginPage components to typescript
  • Add LoginPage to demo app
  • Add integration tests for LoginPage

fixes #1995

@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:login-page-ts branch from f9c9a13 to 83debda May 10, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 10, 2019

Codecov Report

Merging #1967 into master will decrease coverage by 0.42%.
The diff coverage is 65.32%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1967      +/-   ##
=========================================
- Coverage   82.33%   81.9%   -0.43%     
=========================================
  Files         628     628              
  Lines        6980    7053      +73     
  Branches      136     201      +65     
=========================================
+ Hits         5747    5777      +30     
- Misses       1161    1167       +6     
- Partials       72     109      +37
Flag Coverage Δ
#patternfly3 84.88% <ø> (ø) ⬆️
#patternfly4 77.86% <65.32%> (-0.88%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...-core/src/components/LoginPage/LoginFooterItem.tsx 100% <100%> (ø)
.../components/LoginPage/LoginMainFooterLinksItem.tsx 100% <100%> (ø)
...c/components/LoginPage/LoginMainFooterBandItem.tsx 100% <100%> (ø)
.../react-core/src/components/LoginPage/LoginForm.tsx 55.17% <40.9%> (ø)
...eact-core/src/components/LoginPage/LoginHeader.tsx 44.44% <44.44%> (ø)
.../react-core/src/components/LoginPage/LoginPage.tsx 63.63% <45%> (ø)
...ly-4/react-core/src/components/LoginPage/Login.tsx 60% <50%> (ø)
...eact-core/src/components/LoginPage/LoginFooter.tsx 50% <50%> (ø)
...-core/src/components/LoginPage/LoginMainFooter.tsx 53.84% <50%> (ø)
...ct-core/src/components/LoginPage/LoginMainBody.tsx 87.5% <87.5%> (ø)
... and 10 more

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 4972cc3...fb2e632. Read the comment docs.

@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:login-page-ts branch from 83debda to 33f9fb0 May 13, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented May 13, 2019

@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:login-page-ts branch from 33f9fb0 to c7e93de May 13, 2019
footer = null,
header = null,
...props
}) => (

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginProps) => (

@@ -33,7 +29,4 @@ const Login = ({ className, children, footer, header, ...props }) => (
</div>
);

Login.propTypes = propTypes;
Login.defaultProps = defaultProps;

export default Login;

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Remove all default exports.

className = '',
children = null,
...props
}) => (

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginFooterProps) => (

href = '#',
target = '_blank',
...props
}) => {

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginFooterItemProps) => {

onChangeRememberMe = () => undefined,
rememberMeAriaLabel = '',
...props
}) => {

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginFormProps) => ( and ditch the return.

children = null,
className = '',
...props
}) => (

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginMainFooterBandItemProps) => (

target = '',
className = '',
...props
}) => (

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginMainFooterLinksItemProps) => (

title = '',
subtitle = '',
...props
}) => (

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginMainHeaderProps) => (

socialMediaLoginContent,
signUpForAccountMessage = null,
forgotCredentials = null,
socialMediaLoginContent = null,
...props
}) => {

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Change to }: LoginPageProps) => {

.then(userNameInput => expect(userNameInput.text()).to.equal(''));
cy.get('input[name="pf-login-password-id"]')
.then(passwordInput => expect(passwordInput.text()).to.equal(''));
cy.get('#pf-login-remember-me-id')

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Great tests again. Thanks.

@@ -7,6 +7,6 @@ import LinkPreview from '@content/../LinkPreview';
import RawSimpleLoginPage from '!!raw-loader!./examples/SimpleLoginPage';

This comment has been minimized.

Copy link
@dtaylor113

dtaylor113 May 15, 2019

Member

Do you need: typescript: true to show the TS badge?

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Yep!

Copy link
Contributor

tlabaj left a comment

Can you please link the related issue here

@tlabaj tlabaj added the chore 🏠 label May 15, 2019
@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:login-page-ts branch 2 times, most recently from 28a3144 to ba9d441 May 15, 2019
filter: PropTypes.string
})
]),
backgroundImgSrc?: string | BackgroundImageSrcMap;

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

I am not sure this will work for documentation due to the string in the union. We should update this to use the string values for BackgroundImageSrcMap.

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 May 17, 2019

Author Member

@tlabaj My assumption, based on the code in BackgroundImage, is that there are potentially other strings that might be valid as wel (otherwise, why have the string | ?). I can change the BackgroundImageSrcMap part to list the options, but with any string being valid it will still show as union in the docs, correct?

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 17, 2019

Contributor

Ye, that is my understanding as well.

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 May 17, 2019

Author Member

so is it worth listing the values here? If it's going to say union anyhow, might as well use the enum here (I assume we would be using the enums everywhere if the docs worked correctly).

Copy link
Contributor

tlabaj left a comment

LGTM

@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented May 17, 2019

@jeff-phillips-18 Looks good but there's a few conflicts now.

@jeff-phillips-18 jeff-phillips-18 dismissed stale reviews from dlabaj, dtaylor113, and tlabaj via fb2e632 May 17, 2019
@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:login-page-ts branch from ba9d441 to fb2e632 May 17, 2019
@jeff-phillips-18

This comment has been minimized.

Copy link
Member Author

jeff-phillips-18 commented May 17, 2019

Rebased

@dlabaj dlabaj requested a review from redallen May 17, 2019
@dlabaj dlabaj dismissed redallen’s stale review May 17, 2019

Rerequesting review after updates.

@dlabaj
dlabaj approved these changes May 17, 2019
@tlabaj
tlabaj approved these changes May 17, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 4657da1 into patternfly:master May 17, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.