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

John conroy/update workspace templates #3350

Merged
merged 32 commits into from
Dec 1, 2023

Conversation

john-conroy
Copy link
Collaborator

@john-conroy john-conroy commented Nov 30, 2023

Towards HPM-465.

A few known issues:

  1. The API returns successful after a PUT request is made to update templates, but the workspace hasn't been updated to reflect the new templates by the time the subsequent mutate request returns to refresh the workspace data. We will likely have to handle this using SWR with an optimistic UI approach.
  2. '(Optional)' is in the dialog subtitles.
Screenshot 2023-12-01 at 4 55 13 PM Screenshot 2023-12-01 at 4 55 25 PM Screenshot 2023-12-01 at 4 55 39 PM Screenshot 2023-12-01 at 4 55 47 PM

@john-conroy john-conroy changed the base branch from john-conroy/update-workspace-name to main December 1, 2023 16:16
@john-conroy john-conroy marked this pull request as ready for review December 1, 2023 19:16
Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

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

A few small questions 👍🏻

Comment on lines 41 to 44
} finally {
reset();
close();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this finally clause execute even if handleSubmit fails? Should we catch errors here as well, even if we don't do anything with them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the error handling outside of the fetch hook and into the submit callback using a similar recipe to the one used in the Copy button from an earlier PR.

context/app/static/js/shared-styles/surfaces/Step/Step.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

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

👑

@john-conroy john-conroy merged commit dae54d1 into main Dec 1, 2023
8 checks passed
@john-conroy john-conroy deleted the john-conroy/update-workspace-templates branch December 1, 2023 22:09
lchoy pushed a commit that referenced this pull request Dec 13, 2023
* Add hooks to update workspaces

* Add store to control edit dialog

* Create basic workspace name dialog

* Reset form on open

* Handle disabled button tooltip

* Pass mouse events to tooltip button

* Fix update types

* Handle update errors

* Dedupe workspace stores

* Share workspace form fields

* Jump through typescript hoops

* Add step styling to modal

* Pull template select step into own component

* Pull out create templates hook for reuse

* Add edit templates dialog

* Match templates in current workspace details

* Add comment

* Disable templates already in workspace

* Do not select disabled templates when selecting all

* Add tooltip if template is already in workspace

* Create reusable edit workspace dialog

* Move workspace templates and datasets into swr hook

* Handle reset after submit successful

* Consolidate edit dialogs

* Add changelog

* Pass workspace datasets to new templates

* Fix TS errors related to template form types (#3353)

* Update context/app/static/js/shared-styles/cards/SelectableCard/SelectableCard.tsx

Co-authored-by: Nikolay Akhmetov <[email protected]>

* Use store instead of props

* Use padding prop in stack in step

* Use promise chain to avoid async submit callback

* Console error error

---------

Co-authored-by: John Conroy <[email protected]>
Co-authored-by: Nikolay Akhmetov <[email protected]>
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