Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Convert modal to jsx#3649

Merged
jasonLaster merged 4 commits into
firefox-devtools:masterfrom
nyrosmith:convert_modal_to_jsx
Aug 14, 2017
Merged

Convert modal to jsx#3649
jasonLaster merged 4 commits into
firefox-devtools:masterfrom
nyrosmith:convert_modal_to_jsx

Conversation

@nyrosmith
Copy link
Copy Markdown
Contributor

Associated Issue: #3506

Summary of Changes

Changed Modal to JSX syntax

I am quite unsure about the "Slide"-component...

import _Transition from "react-transition-group/Transition";
const Transition = createFactory(_Transition);

import Transition from "react-transition-group/Transition";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really unsure.
Transition is used within Slide. Which is the default export.
I did a quick search but couldn't find any usage of Slide, but bc I am a total noob on your codebase this doesnot mean I didn't oversee something

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slide is a component. You can convert the default export to JSX and make this proper change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this:

export default function Slide({
  in: inProp,
  children,
  handleClose
}: SlideProps) {
  return (
    <Transition id={inProp} timeout={175} appear>
      {status => <Modal status={status} handleClose={handleClose}>{children}</Modal>}
    </Transition>
  );
}

@nyrosmith
Copy link
Copy Markdown
Contributor Author

I could comment out the Slide function and See if it breaks the bundle. But this would only work if you do not dynamics require()...

Copy link
Copy Markdown
Contributor

@wldcordeiro wldcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments about JSX for Slide.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 13, 2017

Codecov Report

Merging #3649 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3649      +/-   ##
==========================================
- Coverage   54.51%   54.49%   -0.02%     
==========================================
  Files         120      120              
  Lines        4786     4784       -2     
  Branches      992      992              
==========================================
- Hits         2609     2607       -2     
  Misses       2177     2177
Impacted Files Coverage Δ
src/components/shared/Modal.js 86.66% <100%> (-1.57%) ⬇️

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 3282e9d...f0e2d8a. Read the comment docs.

@nyrosmith
Copy link
Copy Markdown
Contributor Author

@wldcordeiro @jasonLaster Updated based on your suggestions.
Thx for guidance!
Sorry if formatting doesnot fit but I am unable to run prettier...

Copy link
Copy Markdown
Contributor

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@jasonLaster jasonLaster merged commit 9a09984 into firefox-devtools:master Aug 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants