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.

Sign up for GitHub

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

Form editor v2 #579

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Form editor v2 #579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b122155
Form editor v2
chiragchhatrala Sep 23, 2024
743f8d4
fix template test
chiragchhatrala Sep 23, 2024
0e6bec2
setFormDefaults when save
chiragchhatrala Sep 23, 2024
99955ab
fix form cleaning dark mode
chiragchhatrala Sep 23, 2024
5247f40
improvements on open sidebar
chiragchhatrala Sep 23, 2024
87de29a
Merge branch 'main' into 7953e-better-form-editor
chiragchhatrala Sep 23, 2024
cfad61f
UI polish
JhumanJ Sep 23, 2024
f5a0e24
Fix change type button
JhumanJ Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions api/app/Models/Forms/Form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class Form extends Model implements CachableAttributes
'removed_properties',

'title',
'description',
'tags',
'visibility',

Expand Down Expand Up @@ -177,12 +176,6 @@ public function getViewsCountAttribute()
});
}

public function setDescriptionAttribute($value)
{
// Strip out unwanted html
$this->attributes['description'] = Purify::clean($value);
}

public function setSubmittedTextAttribute($value)
{
// Strip out unwanted html
Expand Down
217 changes: 0 additions & 217 deletions api/app/Service/SeoMetaResolver.php
View file
Open in desktop

This file was deleted.

47 changes: 47 additions & 0 deletions api/database/migrations/2024_09_23_054851_add_description_to_form_properties.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php

use App\Models\Forms\Form;
use Illuminate\Database\Migrations\Migration;
use Illuminate\Support\Str;

return new class () extends Migration {
/**
* Run the migrations.
*/
public function up(): void
{
Form::chunk(100, function ($forms) {
foreach ($forms as $form) {
$properties = $form->properties;
if (!empty($form->description)) {
array_unshift($properties, [
'type' => 'nf-text',
'content' => $form->description,
'name' => 'Description',
'id' => Str::uuid()
]);
$form->properties = $properties;
$form->timestamps = false;
$form->save();
}
}
});
}
Comment on lines +11 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a check for existing 'Description' property

The up() method adds a new 'Description' property to the form's properties array if a description exists. However, there's no check to prevent adding duplicate 'Description' properties if the migration is run multiple times.

Consider adding a check to ensure the 'Description' property doesn't already exist before adding it. Here's a suggested modification:

 public function up(): void
 {
     Form::chunk(100, function ($forms) {
         foreach ($forms as $form) {
             $properties = $form->properties;
-            if (!empty($form->description)) {
+            if (!empty($form->description) && !$this->hasDescriptionProperty($properties)) {
                 array_unshift($properties, [
                     'type' => 'nf-text',
                     'content' => $form->description,
                     'name' => 'Description',
                     'id' => Str::uuid()
                 ]);
                 $form->properties = $properties;
                 $form->timestamps = false;
                 $form->save();
             }
         }
     });
 }

+private function hasDescriptionProperty(array $properties): bool
+{
+    return !empty($properties) && $properties[0]['type'] === 'nf-text' && $properties[0]['name'] === 'Description';
+}

This change ensures that the 'Description' property is only added if it doesn't already exist, preventing potential duplicates.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function up(): void
{
Form::chunk(100, function ($forms) {
foreach ($forms as $form) {
$properties = $form->properties;
if (!empty($form->description)) {
array_unshift($properties, [
'type' => 'nf-text',
'content' => $form->description,
'name' => 'Description',
'id' => Str::uuid()
]);
$form->properties = $properties;
$form->timestamps = false;
$form->save();
}
}
});
}
public function up(): void
{
Form::chunk(100, function ($forms) {
foreach ($forms as $form) {
$properties = $form->properties;
if (!empty($form->description) && !$this->hasDescriptionProperty($properties)) {
array_unshift($properties, [
'type' => 'nf-text',
'content' => $form->description,
'name' => 'Description',
'id' => Str::uuid()
]);
$form->properties = $properties;
$form->timestamps = false;
$form->save();
}
}
});
}
private function hasDescriptionProperty(array $properties): bool
{
return !empty($properties) && $properties[0]['type'] === 'nf-text' && $properties[0]['name'] === 'Description';
}


/**
* Reverse the migrations.
*/
public function down(): void
{
Form::chunk(100, function ($forms) {
foreach ($forms as $form) {
$properties = $form->properties;
if (!empty($properties) && $properties[0]['type'] === 'nf-text' && $properties[0]['name'] === 'Description') {
array_shift($properties);
$form->properties = $properties;
$form->save();
}
}
});
}
Comment on lines +34 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistent timestamp handling in down() method

The down() method doesn't disable timestamps when saving the form, unlike the up() method. This inconsistency could lead to unintended updates to the updated_at field when reverting the migration.

Consider disabling timestamps in the down() method to maintain consistency with the up() method. Here's a suggested modification:

 public function down(): void
 {
     Form::chunk(100, function ($forms) {
         foreach ($forms as $form) {
             $properties = $form->properties;
             if (!empty($properties) && $properties[0]['type'] === 'nf-text' && $properties[0]['name'] === 'Description') {
                 array_shift($properties);
                 $form->properties = $properties;
+                $form->timestamps = false;
                 $form->save();
             }
         }
     });
 }

This change ensures that the updated_at field isn't modified when reverting the migration, maintaining consistency with the up() method.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function down(): void
{
Form::chunk(100, function ($forms) {
foreach ($forms as $form) {
$properties = $form->properties;
if (!empty($properties) && $properties[0]['type'] === 'nf-text' && $properties[0]['name'] === 'Description') {
array_shift($properties);
$form->properties = $properties;
$form->save();
}
}
});
}
public function down(): void
{
Form::chunk(100, function ($forms) {
foreach ($forms as $form) {
$properties = $form->properties;
if (!empty($properties) && $properties[0]['type'] === 'nf-text' && $properties[0]['name'] === 'Description') {
array_shift($properties);
$form->properties = $properties;
$form->timestamps = false;
$form->save();
}
}
});
}

};
6 changes: 2 additions & 4 deletions api/tests/Feature/Forms/FormTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@

$this->assertDatabaseHas('forms', [
'id' => $form->id,
'title' => $form->title,
'description' => $form->description,
'title' => $form->title
]);
});

Expand Down Expand Up @@ -125,8 +124,7 @@
expect($workspace->forms()->count())->toBe(2);
$this->assertDatabaseHas('forms', [
'id' => $response->json('new_form.id'),
'title' => 'Copy of ' . $form->title,
'description' => $form->description,
'title' => 'Copy of ' . $form->title
]);
});

Expand Down
1 change: 0 additions & 1 deletion api/tests/Unit/TestHelpersTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,5 @@
$workspace = $this->createUserWorkspace($user);
$form = $this->makeForm($user, $workspace);
expect($form->title)->not()->toBeNull();
expect($form->description)->not()->toBeNull();
expect(count($form->properties))->not()->toBe(0);
});
12 changes: 11 additions & 1 deletion client/app.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
export default defineAppConfig({
ui: {
primary: 'blue',
gray: 'slate'
gray: 'slate',

tabs: {
wrapper:'space-y-0',
list: {
height: 'h-auto',
tab: {
height: 'h-[30px]'
}
}
}
Comment on lines +6 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

Good addition of tab styling, but consider flexibility

The addition of the tabs object with its nested properties is a good enhancement to the UI configuration. It provides a clear structure for customizing tab styles.

However, I have a suggestion regarding the tab height:

Consider using a more flexible approach for the tab height. The current fixed height of 'h-[30px]' might cause issues with responsiveness or accessibility. You could:

  1. Use a relative unit like rem or em instead of pixels.
  2. Use min-height instead of a fixed height.
  3. Allow for dynamic height based on content.

Here's an example of how you might adjust this:

 tabs: {
   wrapper:'space-y-0',
   list: {
     height: 'h-auto',
     tab: {
-      height: 'h-[30px]'
+      height: 'min-h-[2rem]'
     }
   }
 }

This change would maintain a minimum height while allowing the tab to grow if needed.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
tabs: {
wrapper:'space-y-0',
list: {
height: 'h-auto',
tab: {
height: 'h-[30px]'
}
}
}
tabs: {
wrapper:'space-y-0',
list: {
height: 'h-auto',
tab: {
height: 'min-h-[2rem]'
}
}
}

}
})
6 changes: 5 additions & 1 deletion client/components/forms/ColorInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@
:name="name"
>
<slot name="label">
<span>{{ label }}
<span
:class="[
theme.SelectInput.fontSize,
]"
>{{ label }}
<span
v-if="required"
class="text-red-500 required-dot"
Expand Down
2 changes: 1 addition & 1 deletion client/components/forms/DateInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ const formattedDate = (value) => {
try {
return format(new Date(value), props.dateFormat + (props.timeFormat == 12 ? ' p':' HH:mm'))
} catch (e) {
console.log('Error formatting date', e)
console.error('Error formatting date', e)
return ''
}
}
Expand Down
Loading
Loading