#7432: support placeholder text in form builder #7433

grahamlangford · 2024-01-25T17:10:47Z

What does this PR do?

Closes Support Placeholder Text in Form Builder #7432
Are there other changes (e.g., refactoring) included that aren’t from the ticket?

Demo

https://www.loom.com/share/9c0a387205a94073b36ffe0a692eb8a8

Checklist

Add tests
Designate a primary reviewer @BLoe

codecov · 2024-01-25T17:22:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ed0e25b) 72.61% compared to head (e8d43a4) 72.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7433      +/-   ##
==========================================
+ Coverage   72.61%   72.64%   +0.02%     
==========================================
  Files        1210     1210              
  Lines       37821    37837      +16     
  Branches     7098     7107       +9     
==========================================
+ Hits        27464    27486      +22     
+ Misses      10357    10351       -6

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

fungairino · 2024-01-25T17:27:46Z

src/components/formBuilder/edit/FieldEditor.tsx

@@ -67,6 +67,31 @@ const UNKNOWN_OPTION: SelectStringOption = {
  value: null,
 };

+function showPlaceholderText(uiType: UiType): boolean {


nit: in terms of naming practices I like to lean towards a question style naming for methods that return a boolean, ex. shouldShowPlaceholderText

In terms of re-usability, we might want to consider how we could bundle inherent logic about the uiType into the uiType data structure itself rather than using a helper switch/case function based on its propertyType. This way its easier to refactor and add new uiTypes.

Can't do it with uiType without completely rewriting several files. It's just not worth it at this time.

The problem is this code:

pixiebrix-extension/src/components/formBuilder/edit/FieldEditor.tsx

Lines 148 to 184 in ed0e25b

const getSelectedUiTypeOption = () => {

const fieldSchema = schema.properties[propertyName];

if (typeof fieldSchema === "boolean") {

return UNKNOWN_OPTION;

}

const isDatabaseFieldType = isDatabaseField(fieldSchema);

const isGoogleSheetFieldType = isGoogleSheetIdField(fieldSchema);

const propertyType =

isDatabaseFieldType || isGoogleSheetFieldType

? "string"

: (propertySchema.type as SchemaPropertyType);

const uiWidget = isDatabaseFieldType

? "database"

: isGoogleSheetFieldType

? "googleSheet"

: uiSchema?.[propertyName]?.[UI_WIDGET];

const propertyFormat = propertySchema.format;

const extra: UiTypeExtra =

uiWidget === "select" && propertySchema.oneOf !== undefined

? "selectWithLabels"

: undefined;

const uiType = stringifyUiType({

propertyType,

uiWidget,

propertyFormat,

extra,

});

const selected = fieldTypes.find((option) => option.value === uiType);

return selected ?? UNKNOWN_OPTION;

};

We would need to completely refactor how uiType is inferred. I spent close to 2 hours trying to find a clean way to do it without rewriting multiple components/files and wasn't making much headway, so I pulled out.

Thank you for the effort! It might be good to document this complexity somewhere for posterity, or record a tech debt item/issue.

fungairino · 2024-01-25T17:36:40Z

src/components/formBuilder/edit/FieldEditor.tsx

@@ -299,6 +333,8 @@ const FieldEditor: React.FC<{
          <SchemaField {...defaultFieldProps} />
        )}

+      {showPlaceholderText(uiType) && <SchemaField {...placeholderProps} />}


question: With this change a user will be able to save a placeholder for a particular input field, right? No additional work required for changing how we save the brick.

Yep, this is using the uiSchema part of RJSF, so the actual placeholder valued making it into the real form field when the brick runs should all be handled by RJSF.

fungairino · 2024-01-25T17:37:21Z

src/components/formBuilder/edit/FormEditor.test.tsx

+    "Email",
+    "Website",
+    "Number",
+  ])("displays the placeholder field for %s", async (inputType: string) => {


nit: is there a better type than string for inputType?

inputType is just the string input values in the test.each() call

Right, I was wondering if there's maybe an Enum string type for the possible list of input types

They are just human-readable labels for the test cases

Hmm, although, I guess now that you mention it, it's not totally clear how these test cases map to the different UiTypes... 🤔

See formFieldTypeOptions.ts. I could create some logic to pluck the specific labels.

BLoe · 2024-01-25T17:47:02Z

src/components/formBuilder/edit/FieldEditor.tsx

@@ -67,6 +67,31 @@ const UNKNOWN_OPTION: SelectStringOption = {
  value: null,
 };

+function showPlaceholderText(uiType: UiType): boolean {
+  switch (true) {


switch(true) ?

Oh, is this equivalent to using switch with no input in other languages?

Yes. That said, I'm working on a refactor that should replace the. need for this. And clean things up quite a bit. If I can parse some of the really strange behavior I'm seeing with the uiType dropdown.

github-actions · 2024-01-25T20:21:58Z

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.

grahamlangford added 5 commits January 25, 2024 10:17

adds placeholder field to form builder

390b93f

fix order of fields

8f8caf8

conditionally render placeholder field

7691017

update snapshots

d0f67c5

adds test coverage

49fbaee

grahamlangford linked an issue Jan 25, 2024 that may be closed by this pull request

Support Placeholder Text in Form Builder #7432

Closed

grahamlangford requested review from BLoe, fungairino and mnholtz January 25, 2024 17:11

snapshot update

493edab

vercel bot deployed to Preview January 25, 2024 17:16 View deployment

fungairino reviewed Jan 25, 2024

View reviewed changes

BLoe reviewed Jan 25, 2024

View reviewed changes

make it clearer where the placeholder fields come from in the test

e8d43a4

vercel bot deployed to Preview January 25, 2024 18:58 View deployment

BLoe approved these changes Jan 25, 2024

View reviewed changes

grahamlangford merged commit c976c5e into main Jan 26, 2024
25 checks passed

grahamlangford deleted the 7432-support-placeholder-text-in-form-builder branch January 26, 2024 16:26

grahamlangford added this to the 1.8.8 milestone Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#7432: support placeholder text in form builder #7433

#7432: support placeholder text in form builder #7433

grahamlangford commented Jan 25, 2024 •

edited

codecov bot commented Jan 25, 2024 •

edited

fungairino Jan 25, 2024

fungairino Jan 25, 2024

grahamlangford Jan 25, 2024

grahamlangford Jan 25, 2024

grahamlangford Jan 25, 2024

fungairino Jan 25, 2024

fungairino Jan 25, 2024

BLoe Jan 25, 2024

fungairino Jan 25, 2024

BLoe Jan 25, 2024

fungairino Jan 25, 2024

BLoe Jan 25, 2024

BLoe Jan 25, 2024

grahamlangford Jan 25, 2024

BLoe Jan 25, 2024

BLoe Jan 25, 2024

grahamlangford Jan 25, 2024

github-actions bot commented Jan 25, 2024

	const getSelectedUiTypeOption = () => {
	const fieldSchema = schema.properties[propertyName];
	if (typeof fieldSchema === "boolean") {
	return UNKNOWN_OPTION;
	}

	const isDatabaseFieldType = isDatabaseField(fieldSchema);
	const isGoogleSheetFieldType = isGoogleSheetIdField(fieldSchema);

	const propertyType =
	isDatabaseFieldType \|\| isGoogleSheetFieldType
	? "string"
	: (propertySchema.type as SchemaPropertyType);

	const uiWidget = isDatabaseFieldType
	? "database"
	: isGoogleSheetFieldType
	? "googleSheet"
	: uiSchema?.[propertyName]?.[UI_WIDGET];

	const propertyFormat = propertySchema.format;
	const extra: UiTypeExtra =
	uiWidget === "select" && propertySchema.oneOf !== undefined
	? "selectWithLabels"
	: undefined;

	const uiType = stringifyUiType({
	propertyType,
	uiWidget,
	propertyFormat,
	extra,
	});

	const selected = fieldTypes.find((option) => option.value === uiType);

	return selected ?? UNKNOWN_OPTION;
	};

#7432: support placeholder text in form builder #7433

#7432: support placeholder text in form builder #7433

Conversation

grahamlangford commented Jan 25, 2024 • edited

What does this PR do?

Demo

Checklist

codecov bot commented Jan 25, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2024

grahamlangford commented Jan 25, 2024 •

edited

codecov bot commented Jan 25, 2024 •

edited