#9492 Add database to Rich Text Editor form builder field #9495

mnholtz · 2024-11-12T01:05:14Z

What does this PR do?

Closes Slice 1: Add required database field to Rich Text form builder fields #9492
Adds a required database field to the Rich Text form builder field
Throws a form validation error if a database isn't configured
Includes some very light refactoring of FieldEditor.tsx to extract some code (that file is a bit big)

Discussion

An alternative approach to throwing an error is to silently disable the image upload feature, i.e. make the database field optional

Demo

Demo & product question
https://www.loom.com/share/b38a0cf4304040f4b0bc1b1066c16dd6

mnholtz · 2024-11-12T01:06:03Z

An alternative approach to throwing an error is to silently disable the image upload feature, i.e. make the database field optional

@Pashaminkovsky any preference on the approach here? See demo video for context

github-actions · 2024-11-12T01:19:50Z

Playwright test results

147 passed
7 flaky
2 skipped

Details

Open report ↗︎
156 tests across 51 suites
11 minutes, 47 seconds
f827eca
For more information on how to debug and view this report, see our readme

Flaky tests

msedge-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
chrome › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration
msedge › tests/pageEditor/moveOrCopyModComponent.spec.ts › Create new mod by copying a mod component
msedge › tests/runtime/screenshotTab.spec.ts › screenshot tab brick functionality
msedge › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › can open sidebar immediately from iframe without focus dialog
msedge › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
msedge › tests/telemetry/errors.spec.ts › can report an indexdb error to telemetry service

Skipped tests

chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options

twschiller · 2024-11-12T01:31:39Z

An alternative approach to throwing an error is to silently disable the image upload feature, i.e. make the database field optional

Ideally, the field would be optional and would just disable image upload support if not provided. But for 2.2.0 it's fine if it's required (because it's the primary launch use case for the Rich Text Field)

Pashaminkovsky · 2024-11-12T13:40:45Z

+1 to Todd's comment; let's go with the simplest implementation for now (database is required).

mnholtz · 2024-11-12T16:14:57Z

Ideally, the field would be optional and would just disable image upload support if not provided. But for 2.2.0 it's fine if it's required (because it's the primary launch use case for the Rich Text Field)

I propose that we merge as-is and I can always easily make the field optional once I go to implement the feature

codecov · 2024-11-12T16:19:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.84%. Comparing base (8318d74) to head (f827eca).
Report is 491 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9495      +/-   ##
==========================================
+ Coverage   74.24%   75.84%   +1.59%     
==========================================
  Files        1332     1415      +83     
  Lines       40817    42793    +1976     
  Branches     7634     7884     +250     
==========================================
+ Hits        30306    32455    +2149     
+ Misses      10511    10338     -173

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twschiller · 2024-11-12T16:24:49Z

applications/browser-extension/src/components/formBuilder/edit/fieldEditor/RichTextFields.tsx

+      isRequired
+      schema={{
+        $ref: "https://app.pixiebrix.com/schemas/database#",
+        title: "Database",


Can we change the title to "Asset Database"?

twschiller · 2024-11-12T16:27:44Z

applications/browser-extension/src/components/formBuilder/widgets/RichTextWidget.tsx

+}) => {
+  const { database } = options;
+
+  if (!database) {


I don't like using an error in the field preview as a way of showing an error the user has not provided a value. I prefer we stick with showing it as a field error annotation in the Brick Configuration pane. (I think you'd add as a check in the analysis here:

pixiebrix-extension/applications/browser-extension/src/analysis/analysisVisitors/formBrickAnalysis.ts

Line 28 in 971d00b

class FormBrickAnalysis extends AnalysisVisitorABC {

)

Was there a specific reason you chose to show it in the preview/rendered form instead?

I won't block this PR on it though to keep the ball rolling

Was there a specific reason you chose to show it in the preview/rendered form instead?

I wanted to make sure at minimum that the form displayed errors at run-time due to consideration for passing in variables, but I agree that also including the error annotation in the form itself is ideal.

However - at this point, I'm more convinced that we should favor the approach of making the field optional in the next slice, as it will 1) be easy to do, 2) be more compatible with putting asset database creation behind a feature flag, and 3) reduce complexity around handling form errors.

Let's merge as-is, and I'll remove the required field limitation in favor of disabling the image feature in the next slice.

twschiller · 2024-11-12T16:28:25Z

applications/browser-extension/src/components/formBuilder/edit/fieldEditor/FieldEditor.tsx

@@ -95,60 +96,6 @@ function shouldShowPlaceholderText(uiType: UiType): boolean {
  }
 }

-const TextAreaFields: React.FC<{ uiOptionsPath: string }> = ({


twschiller

See comment here: https://github.com/pixiebrix/pixiebrix-extension/pull/9495/files#r1838408454

Why not use our existing field error annotations?

twschiller

See comment: #9495 (review)

github-actions · 2024-11-12T16:30:59Z

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

mnholtz added 5 commits November 11, 2024 16:09

move FieldEditor into fieldEditor/ subdirectory

9ba2a0c

import TextAreaFields into FieldEditor

e4a290f

add RichTextFields component

4a003d7

get database configuration from options

3586503

type cast and add comment

b054633

mnholtz requested review from twschiller, grahamlangford and Pashaminkovsky November 12, 2024 01:05

twschiller assigned mnholtz Nov 12, 2024

twschiller added the enhancement New feature or request label Nov 12, 2024

twschiller added this to the 2.2.0 milestone Nov 12, 2024

extend rich text widget test timeout

217d880

vercel bot deployed to Preview November 12, 2024 16:12 View deployment

mnholtz requested a review from fungairino November 12, 2024 16:13

twschiller reviewed Nov 12, 2024

View reviewed changes

twschiller self-requested a review November 12, 2024 16:30

twschiller approved these changes Nov 12, 2024

View reviewed changes

rename Database -> Asset Database

f827eca

vercel bot deployed to Preview November 12, 2024 17:12 View deployment

mnholtz merged commit e80e6e6 into main Nov 12, 2024
21 checks passed

mnholtz deleted the feature/9492_req_database_field branch November 12, 2024 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9492 Add database to Rich Text Editor form builder field #9495

#9492 Add database to Rich Text Editor form builder field #9495

mnholtz commented Nov 12, 2024 •

edited

Loading

mnholtz commented Nov 12, 2024 •

edited

Loading

github-actions bot commented Nov 12, 2024 •

edited

Loading

twschiller commented Nov 12, 2024 •

edited

Loading

Pashaminkovsky commented Nov 12, 2024

mnholtz commented Nov 12, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

twschiller Nov 12, 2024

twschiller Nov 12, 2024 •

edited

Loading

mnholtz Nov 12, 2024 •

edited

Loading

twschiller Nov 12, 2024

twschiller left a comment

twschiller left a comment

github-actions bot commented Nov 12, 2024

#9492 Add database to Rich Text Editor form builder field #9495

#9492 Add database to Rich Text Editor form builder field #9495

Conversation

mnholtz commented Nov 12, 2024 • edited Loading

What does this PR do?

Discussion

Demo

mnholtz commented Nov 12, 2024 • edited Loading

github-actions bot commented Nov 12, 2024 • edited Loading

Playwright test results

Details

twschiller commented Nov 12, 2024 • edited Loading

Pashaminkovsky commented Nov 12, 2024

mnholtz commented Nov 12, 2024

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

twschiller Nov 12, 2024

Choose a reason for hiding this comment

twschiller Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

mnholtz Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

twschiller Nov 12, 2024

Choose a reason for hiding this comment

twschiller left a comment

Choose a reason for hiding this comment

twschiller left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 12, 2024

mnholtz commented Nov 12, 2024 •

edited

Loading

mnholtz commented Nov 12, 2024 •

edited

Loading

github-actions bot commented Nov 12, 2024 •

edited

Loading

twschiller commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

twschiller Nov 12, 2024 •

edited

Loading

mnholtz Nov 12, 2024 •

edited

Loading