Skip to content

feat: give users ability to duplicate existing workspaces #10195

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

Closed
wants to merge 24 commits into from

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented Oct 10, 2023

Addresses the UI portion of #9923

@Parkreiner Parkreiner changed the title feat:let users duplicate existing workspaces feat: let users duplicate existing workspaces Oct 12, 2023
@Parkreiner Parkreiner changed the title feat: let users duplicate existing workspaces feat: give users ability to duplicate existing workspaces Oct 13, 2023
@Parkreiner
Copy link
Member Author

There's a flaky test for this part of the codebase that seems to have flared up after the recent auth refactor. Going to see what I can do to pinpoint the source

</FormFields>
</FormSection>
<>
{mode === "duplicate" && <DuplicateWarningMessage />}
Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing that changed for the return value is that the component now returns a React Fragment, so that the DuplicateWarningMessage component can conditionally render. Everything else is a change in indentation level

);
};

type ExternalAuthErrors = Record<string, string>;
function getAuthErrors(
Copy link
Member Author

@Parkreiner Parkreiner Oct 13, 2023

Choose a reason for hiding this comment

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

This replaces the useExternalAuthVerification hook. All its values could be derived from existing state, and I'm always cagey about introducing more state, since there's more chances for things to get out of sync


const CreateWorkspacePage: FC = () => {
const organizationId = useOrganizationId();
const [searchParams] = useSearchParams();
Copy link
Member Author

@Parkreiner Parkreiner Oct 13, 2023

Choose a reason for hiding this comment

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

Sorry for all the shuffling around, but I generally think it's better to keep all the hook calls at the top of the component, instead of mingling them with derived values that aren't necessary for other hook calls

Much easier to avoid Rules of Hooks violations

@@ -38,7 +41,7 @@ const isValidValue = (
return true;
};

export const useValidationSchemaForRichParameters = (
export const validateRichParameters = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the name because it felt like it could mislead someone into thinking it's a React hook

* Buttons supported by workspace actions. Canceling, Deleted, and Pending
* should all be associated with disabled states
*/
export type ButtonType =
Copy link
Member Author

Choose a reason for hiding this comment

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

Wanted to get rid of the enum, just because they can have weird edge case behavior at runtime. It also felt like one of the benefits was making it more clear that the majority of these types are synced with the WorkspaceStatus type

<StartButton workspace={workspace} handleAction={handleStart} />
),
[ButtonTypesEnum.starting]: (
update: <UpdateButton handleAction={handleUpdate} />,
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing major changed with the buttonMapping variable. Just got rid of the enum keys

@Parkreiner Parkreiner closed this Oct 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2023
@Parkreiner Parkreiner deleted the mes/workspace-clone branch November 11, 2023 23:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant