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

refactor(app): Standardize modals and pages #1705

Merged
merged 5 commits into from
Jun 18, 2018
Merged

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Jun 15, 2018

overview

This PR is a much more sane attempt at streamlining pages, modals and scroll behavior. Modals are now siblings of the page within a PageWrapper component (position relative).

changelog

  • Page and Modals siblings within PageWrapper
  • Pages take titleBarProps, TitleBar rendered by page. (In anticipation of all pages requiring TitleBar)
  • refactor CalibrateLabware page to retrieve currentLabware at page level. (Necessary to move modals out of page itself)

review requests

Pleas make sure everything renders and behaves as expected.
This was another crazy one, so please keep an eye out for anything that slipped through the cracks or could be better.

@Kadee80 Kadee80 self-assigned this Jun 15, 2018
@Kadee80 Kadee80 requested review from mcous, IanLondon and b-cooper June 15, 2018 17:19
}
export default function PageWrapper (props: Props) {
return (
<main className={styles.relative}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably only have one <main>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, oversight on my part

}

.task {
flex: 1 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this flex rule necessary now that the parent of the <Page> is not display: flex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will test and see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed for the splash

{!deckPopulated && (
<ReviewDeckModal slot={slot} />
)}
<PageWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than import <PageWrapper> into every single page, we should import it into App.js and wrap the <Switch> there with it. We can replace these PageWrappers with React.Fragments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Jun 15, 2018

Codecov Report

Merging #1705 into edge will decrease coverage by 1.26%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #1705      +/-   ##
==========================================
- Coverage   34.91%   33.65%   -1.27%     
==========================================
  Files         339      349      +10     
  Lines        5553     5782     +229     
==========================================
+ Hits         1939     1946       +7     
- Misses       3614     3836     +222
Impacted Files Coverage Δ
components/src/modals/ModalPage.js 100% <ø> (ø) ⬆️
app/src/pages/Calibrate/Instruments.js 0% <ø> (ø) ⬆️
app/src/components/App.js 0% <ø> (ø) ⬆️
app/src/pages/More/Resources.js 0% <ø> (ø) ⬆️
app/src/pages/Run.js 0% <ø> (ø) ⬆️
app/src/components/LabwareCalibration/index.js 0% <0%> (ø) ⬆️
app/src/components/Page/index.js 0% <0%> (ø)
app/src/pages/More/AppSettings.js 0% <0%> (ø) ⬆️
app/src/components/Page/PageWrapper.js 0% <0%> (ø)
app/src/pages/Robots.js 0% <0%> (ø) ⬆️
... and 111 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 02cb7e3...9d68cf2. Read the comment docs.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

}

type Props = ContextRouter & StateProps
type DispatchProps = {onBackClick: () => void}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use () => mixed and you don't need curly braces below

return {
deckPopulated: !!robotSelectors.getDeckPopulated(state)
onBackClick: () => { dispatch(push(url)) }
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above about () => mixed

@Kadee80 Kadee80 merged commit 9070b21 into edge Jun 18, 2018
@Kadee80 Kadee80 deleted the app_refactor-pages-modals branch June 18, 2018 17:00
@mcous mcous restored the app_refactor-pages-modals branch June 18, 2018 18:08
mcous added a commit that referenced this pull request Jun 18, 2018
mcous added a commit that referenced this pull request Jun 18, 2018
@mcous mcous deleted the app_refactor-pages-modals branch June 28, 2018 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project refactor small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants