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

Make project.description optional #5

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

fvsch
Copy link
Contributor

@fvsch fvsch commented Mar 10, 2023

When using the SDK's embedProject and openProject methods, which expect a project definition object, our TypeScript types require the description to be provided.

Turns out that the StackBlitz backend doesn't actually require a description.

In my own tests, having to come up with a description for a simple example, test or reproduction is often a chore. I've definitely used description: '' before as a workaround. I think we can get out of our users's way and make the description field optional.

What do you think?

@fvsch fvsch requested review from sulco, ykmsd and sylwiavargas March 10, 2023 18:00
@fvsch fvsch force-pushed the fvsch/make-project-description-optional branch from 5e6fa35 to 8d5e782 Compare March 10, 2023 18:14
Copy link

@sylwiavargas sylwiavargas left a comment

Choose a reason for hiding this comment

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

I think this totally makes sense.

@fvsch fvsch force-pushed the fvsch/make-project-description-optional branch from 8d5e782 to fb61bc9 Compare March 10, 2023 18:32
@fvsch fvsch force-pushed the fvsch/make-project-description-optional branch from fb61bc9 to 4615fe5 Compare March 10, 2023 18:36
@@ -2,7 +2,7 @@ import type { PROJECT_TEMPLATES } from './constants';

export interface Project {
title: string;
description: string;
description?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌟 Main type change

form.appendChild(createHiddenInput('project[template]', project.template));
addInput('project[title]', title);
if (typeof description === 'string' && description.length > 0) {
addInput('project[description]', description);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a bit of refactoring of the createProjectForm function, but the main functional change is here: conditionally creating an input to send the description in the POST request.

@@ -5,7 +5,7 @@ exports[`createProjectFrameHTML > generates a HTML document string 1`] = `
<html>
<head><title></title></head>
<body>
<form method=\\"POST\\" style=\\"display:none!important;\\" action=\\"https://stackblitz.com/run?embed=1\\" id=\\"sb_run\\"><input type=\\"hidden\\" name=\\"project[title]\\" value=\\"Test Project\\"><input type=\\"hidden\\" name=\\"project[description]\\" value=\\"This is a test\\"><input type=\\"hidden\\" name=\\"project[template]\\" value=\\"javascript\\"><input type=\\"hidden\\" name=\\"project[files][package.json]\\" value=\\"{
<form method=\\"POST\\" style=\\"display:none!important;\\" action=\\"https://stackblitz.com/run?embed=1\\" id=\\"sb_run\\"><input type=\\"hidden\\" name=\\"project[title]\\" value=\\"Test Project\\"><input type=\\"hidden\\" name=\\"project[template]\\" value=\\"javascript\\"><input type=\\"hidden\\" name=\\"project[files][package.json]\\" value=\\"{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those snapshots are a bit hard to read, especially in GitHub diffs, but they're correct as far as I can tell.

@@ -50,7 +50,7 @@ describe('createProjectForm', () => {

// Check input values
expect(value('project[title]')).toBe('My Test Project');
expect(value('project[description]')).toBe(project.description);
expect(value('project[description]')).toBe(undefined);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the description from the default test project.

@fvsch fvsch merged commit b768756 into main Mar 13, 2023
@fvsch fvsch deleted the fvsch/make-project-description-optional branch March 16, 2023 08:30
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