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

fix(app): Fix alignment issues in modals, fix titlebar on page #1719

Merged
merged 6 commits into from
Jun 18, 2018

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Jun 18, 2018

overview

This PR fixes the alignment issues introduced by #1705. Some modal page titlebars had been cropped. This should standardize behavior once and for all. Worth noting labware calibration uses a spinner modal as modal page content, not the SpinnerModalPage component from comp_lib so a little css override in LabwareCalibration was necessary. While I was in there, I made the TitleBar position fixed.

changelog

  • comp_lib update ModalPage styles
  • app fixed titlebar when in page
  • center spinner modal positioning in LabwareCalibration

review requests

Please click though any and all modals in the app, some are not accessible in VS so a Robot is preferred.

  • attach/change pipette modals and titlebars look as expected
  • calibrate modals and titlebars look as expected (get to at least the first jog screen)
  • labware calibration modals look as expected

@Kadee80 Kadee80 added bug app Affects the `app` project ready for review fix PR fixes a bug labels Jun 18, 2018
@Kadee80 Kadee80 self-assigned this Jun 18, 2018
@Kadee80 Kadee80 requested review from mcous and IanLondon June 18, 2018 18:50
@codecov
Copy link

codecov bot commented Jun 18, 2018

Codecov Report

Merging #1719 into edge will decrease coverage by 0.41%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #1719      +/-   ##
==========================================
- Coverage   35.34%   34.92%   -0.42%     
==========================================
  Files         341      344       +3     
  Lines        5594     5921     +327     
==========================================
+ Hits         1977     2068      +91     
- Misses       3617     3853     +236
Impacted Files Coverage Δ
app/src/components/Page/index.js 0% <ø> (ø) ⬆️
...tocol-designer/src/file-data/selectors/commands.js 21.35% <0%> (-2.78%) ⬇️
protocol-designer/src/step-generation/utils.js 97.1% <0%> (-0.68%) ⬇️
protocol-designer/src/steplist/generateSubsteps.js 0% <0%> (ø) ⬆️
...ol-designer/src/top-selectors/substep-highlight.js 0% <0%> (ø) ⬆️
protocol-designer/src/form-types.js 0% <0%> (ø) ⬆️
...ocol-designer/src/components/StepEditForm/index.js 0% <0%> (ø) ⬆️
protocol-designer/src/containers/Alerts.js 0% <0%> (ø) ⬆️
...r/src/step-generation/dispenseUpdateLiquidState.js 100% <0%> (ø) ⬆️
protocol-designer/src/step-generation/aspirate.js 100% <0%> (ø) ⬆️
... and 8 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 0d99ea0...fd50fca. Read the comment docs.

@IanLondon
Copy link
Contributor

@Kadee80 Looks like some padding is missing from the top:

image

image

@@ -15,6 +15,7 @@

--flex-start: {
display: flex;
flex-direction: column;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes --flex-start little unusable for other components (or at least, it's unexpected given its name). Can we put this on the .modal_page style itself instead?

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

💎 tested on robot, all modals, all good

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.

🔷

@Kadee80 Kadee80 merged commit ccf4881 into edge Jun 18, 2018
@Kadee80 Kadee80 deleted the app_fix-modal-alignment branch June 18, 2018 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project bug fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants