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

add play/pause button to device #5

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Conversation

imaitland
Copy link
Collaborator

@imaitland imaitland commented Oct 28, 2024

Basic PR adding play/pause functionality to the device, following PR will hook it up into the /abort endpoint.

open to feedback for icons and language...

33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@imaitland imaitland force-pushed the iainland/add-abort-button branch from 8126d62 to db77619 Compare October 29, 2024 13:42
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the npm audit step. With dependabot it is redundant.

@@ -33,8 +33,8 @@
},
"devDependencies": {
"@playwright/test": "^1.45.3",
Copy link
Member

Choose a reason for hiding this comment

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

Are changes in this file needed/expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, package-lock is generated by running npm install it pins or "freezes" the dependency tree.

Comment on lines -47 to +48
"@remix-run/dev": "^2.10.2",
"@remix-run/testing": "^2.10.3",
"@remix-run/dev": "^2.13.1",
"@remix-run/testing": "^2.13.1",
Copy link
Member

@jamienoss jamienoss Oct 31, 2024

Choose a reason for hiding this comment

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

This doesn't look right, right? Looks like you accidentally downgraded these packages maybe?
Whoops, sorry, I read this too quickly and totally missed the 3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upgraded npm upgrade will upgrade packages.

Copy link
Member

@jamienoss jamienoss left a comment

Choose a reason for hiding this comment

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

  • I have no idea what I'm looking at to review the .tsx file.
  • Looks like you might have possible left in some dependency requirements files that shouldn't have? 🤷
  • I like the look of the button though from the vid-snippet you posted in the description.

I would have approved if it weren't for the package.json and package-lock.json.

@jamienoss
Copy link
Member

jamienoss commented Oct 31, 2024

@imaitland & @amitschang Looking at ssec-jhu/evolver-ng#200, I would think the UI needs a safety measure to account for someone hitting "play" when something is already running. Otherwise, it'll clobber everything (e.g., config on file) and restart.

Opened #7 to account for this if @imaitland you'd prefer to punt.

@imaitland
Copy link
Collaborator Author

  • I have no idea what I'm looking at to review the .tsx file.

    • Looks like you might have possible left in some dependency requirements files that shouldn't have? 🤷

    • I like the look of the button though from the vid-snippet you posted in the description.

I would have approved if it weren't for the package.json and package-lock.json.

Do my comments regarding changes to package.json and package-lock.json make sense? Happy to break them out into a seperate pr, tho i don't think this is necessary here.

@imaitland
Copy link
Collaborator Author

@imaitland & @amitschang Looking at ssec-jhu/evolver-ng#200, I would think the UI needs a safety measure to account for someone hitting "play" when something is already running. Otherwise, it'll clobber everything (e.g., config on file) and restart.

@jamienoss what specifically does it clobber?

My understanding is that sending a POST request to /stop does the following:

  1. toggle config flags:
    config.enable_control = False
    config.enable_commit = False
  2. Save config to evolver.yml

so nothing should be clobbered (except those flags) by stopping and then starting the evolver?

@jamienoss
Copy link
Member

Re: #5 (comment)

That's correct, but only if you abort first. It would be a shame to accidentally click start whilst an experiment is already running and thus restart it.

@imaitland
Copy link
Collaborator Author

Re: #5 (comment)

That's correct, but only if you abort first. It would be a shame to accidentally click start whilst an experiment is already running and thus restart it.

start/stop as implemented in this pr is a toggle. So from a UX pov - you can't start an already running evolver.

User can do whatever they want against the http api.

If there needs to be a guard it should be on the device's /start http endpoint, if there isn't one already - i.e. make it impossible to start an already running evovler.

@jamienoss
Copy link
Member

start/stop as implemented in this pr is a toggle. So from a UX pov - you can't start an already running evolver.

Awesome! 👍

@imaitland imaitland merged commit 89c340f into main Nov 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants