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(Modal): Convert Modal component to typescript #1942

Merged
merged 4 commits into from May 17, 2019

Conversation

@jeff-phillips-18
Copy link
Member

jeff-phillips-18 commented May 7, 2019

What: Convert Modal component to typescript

Additional issues: Allows specification of heading level on the ModalBoxHeader

Fixes #1736, #1996

@jeff-phillips-18 jeff-phillips-18 requested review from jschuler and redallen May 7, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented May 7, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 7, 2019

Codecov Report

Merging #1942 into master will decrease coverage by 0.06%.
The diff coverage is 80.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1942      +/-   ##
==========================================
- Coverage    81.9%   81.84%   -0.07%     
==========================================
  Files         628      628              
  Lines        7053     7112      +59     
  Branches      201      236      +35     
==========================================
+ Hits         5777     5821      +44     
+ Misses       1167     1163       -4     
- Partials      109      128      +19
Flag Coverage Δ
#patternfly3 84.88% <ø> (ø) ⬆️
#patternfly4 77.8% <80.91%> (-0.07%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...4/react-core/src/components/Modal/ModalContent.tsx 67.74% <67.74%> (ø)
...4/react-core/src/components/Modal/ModalBoxBody.tsx 75% <75%> (ø)
...react-core/src/components/Modal/ModalBoxFooter.tsx 75% <75%> (ø)
...-core/src/components/Modal/ModalBoxCloseButton.tsx 75% <75%> (ø)
...react-core/src/components/Modal/ModalBoxHeader.tsx 83.33% <83.33%> (ø)
...ernfly-4/react-core/src/components/Modal/Modal.tsx 88.23% <88.23%> (ø)
...fly-4/react-core/src/components/Modal/ModalBox.tsx 92.3% <92.3%> (ø)
... and 4 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 5f9b092...793484d. Read the comment docs.

@dlabaj dlabaj added the PF4 label May 7, 2019
@nicolethoen

This comment has been minimized.

Copy link
Contributor

nicolethoen commented May 8, 2019

@jeff-phillips-18 can you also add integration tests for this component as outlined on this README?

@redallen redallen added the TypeScript label May 9, 2019
@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:modal-ts branch from da525b0 to 13902ff May 10, 2019
@jeff-phillips-18

This comment has been minimized.

Copy link
Member Author

jeff-phillips-18 commented May 10, 2019

Added integration tests and demo page

@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:modal-ts branch from 13902ff to 2fce11a May 13, 2019
/** A callback for when the close button is clicked */
onClose: PropTypes.func,
onClose?(): void;

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Nitpicky, but we've been doing onClose?: () => void. Sometimes different docgen versions pick it up differently.

container?: HTMLDivElement = undefined;

static defaultProps = {
width: undefined as any,

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Can remove this line, width is by default undefined.

constructor(props: ModalProps) {
super(props);
const newId = Modal.currentId++;
this.id = `pf-modal-${newId}`;

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

For AboutModal, I added this to state for lifecycle purposes instead of just this. Perhaps we should be consistent?

This comment has been minimized.

Copy link
@jeff-phillips-18

jeff-phillips-18 May 15, 2019

Author Member

Personally I prefer it to just be an instance variable than on state. To me, the id is not stateful.

if (event.keyCode === KEY_CODES.ESCAPE_KEY && this.props.isOpen) {
this.props.onClose();
this.props.onClose!();

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Let's set onClose = () => undefined as any in defaultProps (if we can) and avoid the ugly shebang!

}
}
};

componentDidMount() {
document.body.appendChild(this.container);
if (this.container) {

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

I initialized the container here in AboutModal which makes more sense to do than in render() IMO. That way we don't rely on render being called before componentDidMount. See my AboutModal PR.

isLarge: PropTypes.bool,
/** Creates a small version of the Modal */
isSmall: PropTypes.bool,
onClose?(): void;

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

onClose?: () => void;

@@ -94,7 +88,4 @@ const ModalContent = ({
);
};

ModalContent.propTypes = propTypes;
ModalContent.defaultProps = defaultProps;

export default ModalContent;

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Remove this line.

id,
hideTitle = false,
actions = [],
onClose = () => undefined,

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Probably will need to change to () => undefined as any

onClose = () => undefined,
isLarge = false,
isSmall = false,
width = -1,

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

I appreciate this instead of null.

cy.get('#showDefaultModalButton').then((modalButton: JQuery<HTMLButtonElement>) => {
cy.wrap(modalButton).click();
cy.get('.pf-c-modal-box').then(() => {
cy.get('.pf-c-modal-box .pf-c-button[aria-label="Close"]').then(closeButton => {

This comment has been minimized.

Copy link
@redallen

redallen May 15, 2019

Contributor

Great tests. Thanks a lot!

@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:modal-ts branch 3 times, most recently from 9fd7dee to 163d17c May 15, 2019
@tlabaj tlabaj added the chore 🏠 label May 15, 2019
@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:modal-ts branch from 163d17c to d6c773d May 16, 2019
Copy link
Contributor

tlabaj left a comment

Looks great. A couple of small things.

import styles from '@patternfly/patternfly/components/ModalBox/modal-box.css';

export interface ModalBoxProps extends React.HTMLProps<HTMLDivElement> {
/** content rendered inside the ModalBox. */

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

We have been updating our docs to use sentence case. Can you please update these comments.

import styles from '@patternfly/patternfly/components/ModalBox/modal-box.css';

export interface ModalBoxBodyProps extends React.HTMLProps<HTMLDivElement> {
/** content rendered inside the ModalBoxBody */

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

Same comment about Sentence case here and for all other exported interfaces.

/** Flag to hide the title */
hideTitle?: boolean;
/** the heading level to use */
headingLevel?: TitleLevel;

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

We should be using a union of the strings here as was decided upon by the team.

@jeff-phillips-18 jeff-phillips-18 force-pushed the jeff-phillips-18:modal-ts branch from d6c773d to 793484d May 17, 2019
@tlabaj
tlabaj approved these changes May 17, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

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

dlabaj left a comment

Thanks @jeff-phillips-18 looks great!

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

Updated with @redallen comments

@dlabaj dlabaj merged commit d148e91 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.