Skip to content

feat(custom-fields-ugn-master) #221

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

audi2014
Copy link
Contributor

@audi2014 audi2014 commented Nov 14, 2023

Free-form "custom" fields? #83 Follow-up #202

Goal: add ability to create new fields in runtime from unmapped csv columns

Screenshot 2023-11-14 at 14 34 55
  • add customFieldsHook into RsiProps: Runs on Match Columns step for each column when their state did change. Used to define custom fields with keys that are can be generated only from csv column header.
  • implement customFieldsHook logic
  • add dropDownLabel into Field - UI-facing label for dropdown option
Screenshot 2023-11-14 at 14 28 39

Yes, this logic can be implemented by matchColumnsStepHook but, but in this case user has poor UX:

  • can't change column's state from matched to unmatched to mark column as custom field for matchColumnsStepHook
  • hard to implement

Yes, this logic can be implemented by selectHeaderStepHook (use local state for fields and update them here)

  • but in this case all unknown headers will be displayed on each dropdown list in Match Columns Step.

* add customFieldsHook into RsiProps: Runs on Match Columns step for each column when their state did change. Used to define custom fields with keys that are can be generated only from csv column header.
* implement customFieldsHook logic
* add dropDownLabel into Field - UI-facing label for dropdown option
- revert MatchIcon changes
- more generic example of custom fields
- mergeCustomFields - add custom fields to end of fields array
@masiulis
Copy link
Contributor

masiulis commented Nov 21, 2023

Hey @audi2014, thank you for another contribution! Could you expand why similar code to this code doesn't solve the custom field problem:

export const Component = () => {
  const [data, setData] = useState<any>(null)
  const [fields, setFields] = useState<any>([
    {
      label: "Name",
      key: "name",
      alternateMatches: ["first name", "first"],
      fieldType: {
        type: "input",
      },
      example: "Stephanie",
      validations: [
        {
          rule: "required",
          errorMessage: "Name is required",
        },
      ],
    },
  ])
  return (
    <ReactSpreadsheetImport
      fields={fields}
      isOpen={true}
      onClose={() => {}}
      onSubmit={setData}
      selectHeaderStepHook={async (headerValues, data) => {
        // Put logic to figure out the difference between "headerValues" and what is already available in "fields"
        const customFields = ...

        setFields([...fields, ...customFields])

        return data
      }}
    />
  )
}

I understand that this PR adds a custom icon for these fields and an ability to change the column name in the dropdown. But I am not sure why a new hook is needed in this case.

EDIT: I am interested in selectHeaderStepHook not matchColumnsStepHook

@audi2014
Copy link
Contributor Author

audi2014 commented Nov 27, 2023

Hey @audi2014, thank you for another contribution! Could you expand why similar code to this code doesn't solve the custom field problem:

I understand that this PR adds a custom icon for these fields and an ability to change the column name in the dropdown. But I am not sure why a new hook is needed in this case.

EDIT: I am interested in selectHeaderStepHook not matchColumnsStepHook

Hello, @masiulis!

Technically, selectHeaderStepHook covers 100% functionality for the end-user. But from UX perspective this behavior looks wired.

diff --git a/src/stories/Default.stories.tsx b/src/stories/Default.stories.tsx
index ce027f4..76b76bc 100644
--- a/src/stories/Default.stories.tsx
+++ b/src/stories/Default.stories.tsx
@@ -1,16 +1,37 @@
 import { ReactSpreadsheetImport } from "../ReactSpreadsheetImport"
-import { Box, Link, Code, Button, useDisclosure } from "@chakra-ui/react"
+import { Box, Button, Code, Link, useDisclosure } from "@chakra-ui/react"
 import { mockRsiValues } from "./mockRsiValues"
 import { useState } from "react"
 import type { Result } from "src/types"
+import { Field } from "src/types"
 
 export default {
   title: "React spreadsheet import",
 }
 
+const FIELDS: Field<string>[] = [
+  {
+    label: "Name",
+    key: "name",
+    alternateMatches: ["first name", "first"],
+    fieldType: {
+      type: "input",
+    },
+    example: "Stephanie",
+    validations: [
+      {
+        rule: "required",
+        errorMessage: "Name is required",
+      },
+    ],
+  },
+]
+
 export const Basic = () => {
   const [data, setData] = useState<Result<any> | null>(null)
   const { isOpen, onOpen, onClose } = useDisclosure()
+  const [fields, setFields] = useState(FIELDS)
+
   return (
     <>
       <Box py={20} display="flex" gap="8px" alignItems="center">
@@ -22,7 +43,29 @@ export const Basic = () => {
       <Link href="./exampleFile.csv" border="2px solid #718096" p="8px" borderRadius="8px" download="exampleCSV">
         Download example file
       </Link>
-      <ReactSpreadsheetImport {...mockRsiValues} isOpen={isOpen} onClose={onClose} onSubmit={setData} />
+      <ReactSpreadsheetImport
+        {...mockRsiValues}
+        fields={fields}
+        selectHeaderStepHook={async (headerValues, data) => {
+          setFields([
+            ...FIELDS,
+            ...headerValues.map(
+              (e) =>
+                ({
+                  label: "CF:" + e,
+                  key: e,
+                  fieldType: {
+                    type: "input",
+                  },
+                } as Field<string>),
+            ),
+          ])
+          return { headerValues, data }
+        }}
+        isOpen={isOpen}
+        onClose={onClose}
+        onSubmit={setData}
+      />
       {!!data && (
         <Box pt={64} display="flex" gap="8px" flexDirection="column">
           <b>Returned data (showing first 100 rows):</b>

@audi2014 audi2014 closed this Nov 27, 2023
@audi2014 audi2014 reopened this Nov 27, 2023
@audi2014
Copy link
Contributor Author

^ Oh! I accidentally closed this PR

so here the issure:

If the user have CSV file with 40 unknown fields - all these fields will be represented in the dropdown list. Displaying unknown field names for each column is unnecessary from user perspective

Screenshot 2023-11-27 at 17 31 46

@audi2014
Copy link
Contributor Author

audi2014 commented Nov 27, 2023

Second Issue:
All unknown fields displayed in the cell editor
It can be fixed by field filtering in matchColumnsStepHook - but this implementation is not so simple

Screenshot 2023-11-27 at 17 47 53 Screenshot 2023-11-27 at 17 43 13

In general: the nature of custom fields is dynamic. A user wants to have the ability to create subset of custom fields in runtime, but not to crate fields for every unknown field

Also, the user probably wants to create a custom field with some predefined validation or want to map values into boolean checkboxes. All these actions require interaction with user.

But I agree that this functionality looks more like an edge case and my implementation is too complex and flexible.

@masiulis
Copy link
Contributor

@audi2014 I am still trying to wrap my head around the problem from the users perspective. You mentioned:

In general: the nature of custom fields is dynamic. A user wants to have the ability to create subset of custom fields in runtime, but not to crate fields for every unknown field

If we want to give this functionality to the user, maybe allowing marking the column as custom in UI would work?
Screenshot 2023-11-29 at 22 27 15

This would avoid a custom hook, and would not clutter dropdown or table columns. But because I don't fully understand the problem, I don't know if this would fully solve what you are trying to solve.

Also, the user probably wants to create a custom field with some predefined validation or want to map values into boolean checkboxes. All these actions require interaction with user.

I am not sure how would a user define validation or mapping in your case. By user do you mean the end user importing the file, or the programmer configuring the hooks?

@audi2014
Copy link
Contributor Author

audi2014 commented Dec 6, 2023

If we want to give this functionality to the user, maybe allowing marking the column as custom in UI would work? Screenshot 2023-11-29 at 22 27 15

@masiulis sorry for the delay.
If I understand your screenshot correctly, you would like to add a new optional boolean property like "isCustomFieldsEnabled". If this flag is turned on - the app will show the option "+ Add as custom field"

I like this approach. It is simple and predictable. However, I do not know your vision of what should happen next.
Therefore, I will try to describe the continuation of this approach and the problems that need to be solved.

Preconditions:

  • Developer defines 2 fields for RSI: (Title, Body).
  • Developer sets RSI property "isCustomFieldsEnabled={true}"
  • After submitting csv data to API, values of "Title" and "Body" will be saved in the db table "Post" and all other columns will be saved in db table "PostMeta"
  • User uploads CSV with columns (Title, Body, COL_X)
  • User opens "Match Columns" Step
  • RSI cant find a field for CSV column COL_X

Case 1 (Default Positive)

  • User selects "+ Add as custom field" option for "COL_X" column
  • COL_X become a new RSI field with "input" type without any validations
  • Validate data step: user can edit the "Title", "Body" and "COL_X" columns
  • Submit: rows with data like {Title:string, Body:string,COL_X:string}[] will be submitted to API
  • Api: Created a new Post row and related PostMeta

Case 2 (Defined field marked as a custom field)

  • user marks "COL_X" column as ignored
  • User selects "+ Add as custom field" option for "Body" column (predefined field)
  • ??? What we should do in this case?
    • Follow Case 1? In this case "Body" column will be displayed as a custom field on "Match Columns" Step. but after that "Body" column will become as normal none-custom field and will be saved into "Post" db table. So we can't use this approach.
    • Denny to mark predefined fields as custom fields?
    • Add some prefix to "Body" like "__cf__Body". In this case, the app can follow the Case 1 path. But is it a good idea to use magic prefixes? should we add the ability to define this prefix in RSI props?

Case 3 (user wants all unknown fields to be automatically marked as custom fields)

  • can be implemented by adding another RSI property like "autoSelectCustomFields"

In my approach (with customFieldsHook) All these questions can be resolved on the Developer's side:

  • The developer can define how a new column key should be generated to avoid column duplication (Case 2)
  • The developer can define "alternateMatches" for potential custom fields to implement automatic selection of custom fields
  • also developer can define field types and validations

But my approach (with customFieldsHook) is too complicated. So I am 100% agree. Final Solution should be simpler. We cannot complicate the library for 100% of developers to satisfy 1% of edge cases.

@masiulis
Copy link
Contributor

Hey @audi2014, thanks for your detailed response. I have a proposal that I would like to run by you.

  • Add customFieldsAllowed flag.
  • In Match Columns step, all unselected, but not ignored fields automatically become "custom" fields.
  • In Validation steps we automatically add these "custom" columns - you don't need to manually change fields prop.
  • OnSubmit function gets custom fields nested like this {name: John, _customFields: {firstname: 'Johnathan', lastname: 'Doe'}} - this would collide if you had a field prop named _customFields, but we can make this typesafe in TS.

Problems with this solution:

  • In Match Columns step we need to explain to the user what's going on. I would propose changing the icon to an information icon, on hover show text "Selecting a column is not mandatory. If a column is not selected the field will be imported as a custom field. To not import this column click X to ignore it.". We can also add a similar text somewhere at the top.
  • A user has to manually ignore columns it does not want to import, now they can just leave the dropdown empty. I don't think this is a big issue though, if we explain it properly.
  • Custom field validations and types cannot be set - I would assume that simple text fields is good enough in most cases, as by definition you don't know the custom field type.

Comparing with your points:

Case 1 (Default Positive)

Works automatically, user doesn't need to select anything.

Case 2 (Defined field marked as a custom field)

Only unselected fields become custom fields, and because they are in their own namespace _customFields, they wouldn't collide.

can be implemented by adding another RSI property like "autoSelectCustomFields"
In my approach (with customFieldsHook) All these questions can be resolved on the Developer's side:

This would be the default (and only) behaviour.

The developer can define how a new column key should be generated to avoid column duplication (Case 2)

Would not be a problem as all fields would be nested in _customFields

The developer can define "alternateMatches" for potential custom fields to implement automatic selection of custom fields

Would not be a problem as custom fields would be created after match column step, so there is no manual field creation and automatic matching required.

also developer can define field types and validations

That would not be possible not, if this is a big requirement it could be a separate hook then that is ran on custom field creation.

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