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

Suspension Dash GUI #22

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

afhassan
Copy link
Member

@afhassan afhassan commented Sep 13, 2020

This branch contains the GUI for suspension testing. It prompts entering test parameters and allows saving them as profiles in a JSON file. You can load profiles or delete them. The actual testing logic hasn't been done yet.

Features tested:

  • Saving a profile
  • Loading profiles
  • Overwriting an existing profile
  • Deleting a profile

Note: some fixes need to be done for layout sizing

Suspension GUI

Copy link
Member

@colton-smith colton-smith left a comment

Choose a reason for hiding this comment

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

Very sick PR Ahmed !

Couple of requested changes:

  • Use new app.callback() arguement prevent_initial_call = True to prevent the callback from firing initially, and remove the workaround we used before
  • Very few tiny style changes like consistent line spacing, and one or 2 long lines
  • Most importantly, I think we will need a data structure to hold the profile in so that we can access that data when we go to start the test! This could be in a class - all we need is to load the profile (we can already do so) then on start we will have to read all the values in the current input boxes and set them to fields in a class
  • The other important thing is exposing the min/max values a little more using constants at the top of the layout file. This is super easy, just use arbitrary values if they aren't in the spec doc, and we will ask any questions you have in a meeting with Nick

Very close to merge worthy!

@@ -1 +1,18 @@
# Stability dash entrypoint
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see top of file comments replaced with a module docstring

Comment on lines +2 to +13
import time

import dash
import dash_bootstrap_components as dbc
import dash_core_components as dcc
import dash_daq as daq
import dash_html_components as html
from dash.dependencies import Input, Output, State

import stability_dash.callbacks.control_callbacks
from stability_dash.app import app
from stability_dash.layout.control import control_layout
Copy link
Member

Choose a reason for hiding this comment

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

Imports look to be in order, nice~!

@@ -0,0 +1,44 @@
.pad-top {
Copy link
Member

Choose a reason for hiding this comment

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

We (meaning I) am pretty dumb for not laying out these dash apps to all use the same assets folder so we could avoid copy pasting what little custom css we do have

Way she goes

Comment on lines +5 to +6
from dash.dependencies import Input, Output, State
from dash.dash import no_update
Copy link
Member

Choose a reason for hiding this comment

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

Swap these 2 to maintain alphabetical order of imports

json.dump(profiles_json, profiles_file, indent=4)

return profile_name

Copy link
Member

Choose a reason for hiding this comment

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

2 lines


dbc.Row([
dbc.Col([
dbc.Card([dbc.Button("EMERGENCY STOP", color="danger", size="lg", block = True, style={"height":"100%","width":"100%"} )])
Copy link
Member

Choose a reason for hiding this comment

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

This row is a little long - maybe stack some of the arguments to button

@@ -0,0 +1,151 @@
import dash
Copy link
Member

Choose a reason for hiding this comment

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

This file is super clean, nice job dude

Comment on lines +127 to +128
if not delete_clicks:
raise dash.exceptions.PreventUpdate
Copy link
Member

Choose a reason for hiding this comment

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

See above comment about the new feature

Comment on lines +21 to +22


Copy link
Member

Choose a reason for hiding this comment

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

Should be always one space between rows or always 2 spaces, seems like you went with mostly one space so you can just make this one space as well

Comment on lines +9 to +10

dbc.Row([
Copy link
Member

Choose a reason for hiding this comment

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

Add Min/Max constants for each input at the top of this file.

OR we can add another file like suspension_config.py that includes all these constants and then import it. I kind of like that a little better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants