feat(Modal): Convert Modal component to typescript by jeff-phillips-18 · Pull Request #1942 · patternfly/patternfly-react · GitHub

What: Convert Modal component to typescript

Additional issues: Allows specification of heading level on the ModalBoxHeader

Fixes #1736, #1996

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

Codecov Report

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

@@            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.

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

Added integration tests and demo page

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

Can remove this line, width is by default undefined.

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

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

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

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.

onClose?: () => void;

Remove this line.

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

I appreciate this instead of null.

Great tests. Thanks a lot!

Looks great. A couple of small things.

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

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

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

LGTM

Thanks @jeff-phillips-18 looks great!

Updated with @redallen comments

jeff-phillips-18 requested review from jschuler and redallen May 7, 2019 13:41

dlabaj added the PF4 label May 7, 2019

redallen added the TypeScript label May 9, 2019

jeff-phillips-18 force-pushed the modal-ts branch from da525b0 to 13902ff Compare May 10, 2019 17:55

jeff-phillips-18 force-pushed the modal-ts branch from 13902ff to 2fce11a Compare May 13, 2019 12:09

rachael-phillips mentioned this pull request May 13, 2019

Convert Modal to TypeScript #1996

Closed

redallen previously requested changes May 15, 2019

View reviewed changes

jeff-phillips-18 force-pushed the modal-ts branch 3 times, most recently from 9fd7dee to 163d17c Compare May 15, 2019 18:41

tlabaj added the chore 🏠 label May 15, 2019

jeff-phillips-18 force-pushed the modal-ts branch from 163d17c to d6c773d Compare May 16, 2019 18:03

tlabaj suggested changes May 16, 2019

View reviewed changes

jeff-phillips-18 added 4 commits May 17, 2019 10:57

feat(Modal): Convert Modal component to typescript

bfba556

Convert to Typescript add Demo and Test

8d17387

Updates per review comments

e2c1271

Further review comment updates

793484d

jeff-phillips-18 force-pushed the modal-ts branch from d6c773d to 793484d Compare May 17, 2019 15:18

tlabaj approved these changes May 17, 2019

View reviewed changes

dlabaj approved these changes May 17, 2019

View reviewed changes

dlabaj merged commit d148e91 into patternfly:master May 17, 2019

redallen mentioned this pull request Jun 14, 2019

PF4: Modal onClose callbacked changed to () => void; in @patternfly/react-core 3.16.4 and prior it allowed passing an argument #2233

Closed

jeff-phillips-18 deleted the modal-ts branch May 12, 2022 12:00

+                actions?: any,
                 /** A callback for when the close button is clicked */
-                onClose: PropTypes.func,
+                onClose?(): void;

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

+                handleEscKeyClick = (event: KeyboardEvent): void => {
                   if (event.keyCode === KEY_CODES.ESCAPE_KEY && this.props.isOpen) {
-                    this.props.onClose();
+                    this.props.onClose!();

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

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

+                container?: HTMLDivElement = undefined;
+                static defaultProps = {
+                  width: undefined as any,

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

-              ModalContent.propTypes = propTypes;
-              ModalContent.defaultProps = defaultProps;
               export default ModalContent;

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

+                  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 => {

+              import styles from '@patternfly/patternfly/components/ModalBox/modal-box.css';
+              export interface ModalBoxProps extends React.HTMLProps<HTMLDivElement> {
+                /** content rendered inside the ModalBox. */

+              import styles from '@patternfly/patternfly/components/ModalBox/modal-box.css';
+              export interface ModalBoxBodyProps extends React.HTMLProps<HTMLDivElement> {
+                /** content rendered inside the ModalBoxBody */

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

feat(Modal): Convert Modal component to typescript by jeff-phillips-18 · Pull Request #1942 · patternfly/patternfly-react · GitHub

Uh oh!

feat(Modal): Convert Modal component to typescript - #1942

feat(Modal): Convert Modal component to typescript#1942dlabaj merged 4 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom jeff-phillips-18:modal-tsjeff-phillips-18/patternfly-react:modal-tsCopy head branch name to clipboard

Conversation

jeff-phillips-18 commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented May 7, 2019

Uh oh!

codecov-io commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nicolethoen commented May 8, 2019

Uh oh!

jeff-phillips-18 commented May 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

feat(Modal): Convert Modal component to typescript#1942
dlabaj merged 4 commits into
patternfly:masterfrom
jeff-phillips-18:modal-ts

jeff-phillips-18 commented May 7, 2019 •
edited
Loading

codecov-io commented May 7, 2019 •
edited
Loading