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

Support array column type in recordedit #1408

Closed
hongsudt opened this issue Dec 13, 2017 · 17 comments · Fixed by #2340
Closed

Support array column type in recordedit #1408

hongsudt opened this issue Dec 13, 2017 · 17 comments · Fixed by #2340
Assignees
Labels
help wanted Intended for internal use for student workers recordedit

Comments

@hongsudt
Copy link
Contributor

hongsudt commented Dec 13, 2017

The array column type's contents can only be values of scalar types.

In the recordedit form, need to support the following functionality:

  • adding more cells for the array
  • removing cells from the array
  • reordering the data in the array
  • edit any one of the cells

This issue is currently relying on this PR in ermrestJS. It adds more functionality for tinkering with the display of array columns.

NOTE:

  • No array of type markdown.
  • This allows for a third level of types to be defined. (domain type that is of type array that has scalar types inside)
@amjha
Copy link
Contributor

amjha commented Feb 22, 2018

As per today's discussion, 4 different cases pertaining to base type and domain type.

No. isArray isDomain Typename Base type
1 False False Text
2 False True Markdown 1
3 True False Text [ ] 1
4 False True "Tex" 3

@RFSH
Copy link
Member

RFSH commented May 2, 2018

I created an issue for the changes that we need in ermrestjs, to have basic array support in the non-entry modes.

@jrchudy
Copy link
Member

jrchudy commented Jun 1, 2018

  • Types that have yet to be implemented and tested fully:
    • float
    • date
    • timestamp
  • Test cases are very minimal and need to be extended to each of these types.
  • presentation bug when a new cell is added, it duplicates column header.
  • HTML changes (needs more information)

@hongsudt hongsudt added the help wanted Intended for internal use for student workers label Jun 18, 2018
@hongsudt
Copy link
Contributor Author

hongsudt commented Nov 7, 2018

A simple array support that shouldn't take a lot of time is to provide a adjustable text box and validate to make sure that it follows the json array syntax.

@RFSH
Copy link
Member

RFSH commented Dec 21, 2018

Required features and How it should behave

  • Chaise should detect array columns. You can do this check by looking at the Column.type.isArray attribute.
  • When there's no existing data (in create or edit mode), we should show one input alongside two buttons. One for adding new input and one for removing the existing input (each input should have the remove button beside it). The latter should be disabled if there's just one input showing. The ordering of inputs matter too. I don't think this is in the requirements but we might want to add a way to change the ordering of inputs. There are some directives that might give you all these features.
  • When there's an existing value we should put each of the array values into different inputs.
  • Having just one input that is empty should be translated into null value instead of an array of null ([null]).
  • If there are more than one inputs, we should treat the empty ones as null value. So for example if there are two inputs and the first one is empty and the second one has a value of "test". We should send a [null, "test"] to ermrestjs.

How to create array column

Some examples of array column can be found here. You can create your own schema with some array columns.

Guideline and code changes

Before jumping into the code I think it's better if you just take a quick look at how the recordedit is written. It's our most complicated app, so just take a look at the existing code before jumping into the code. Since I'm not very fluent with chaise code, I did the same and the following steps are what I think would be the best way to approach this. You don't need to completely follow them. It's just a guideline. I might have missed some steps.

  • Add a new inputType that this function is going to return. Then the appropriate inputType will be attached to the columnModel in here.

  • Add a new type of input here that will be used when the inputType is what you defined in the last step. It might be better if you create your own directive for array inputs, I'm not sure. At this point you might want to focus more on the UI and make sure all the components are working properly. Then you can work on actually integrating it with recordedit.

  • You most probably should reuse the input-switch directive. It will create appropriate type of input based on its type. It's currently only used for the "select-all" feature. But eventually we might want to refactor the existing input types in recordedit to use that. You most probably need to adjust input-switch too and add the array columns in there. We want the select-all feature for array columns too. It might be easier to ignore adding select-all feature to arrays for now. I'm not sure, you might have to think more about this.

  • Your array input directive should handle adding multiple inputs itself. You might be able to have all the logic of handling the array value types in there without involving any of the existing recordedit code. For example in the form.controller.js in different places we are transforming the value based on its type. If your directive does these sort of logic by itself, then you don't need to worry about changing the existing code. And the existing code can just treat that input like an ordinary input (there won't be any transformation and it will just copy the data. The actual transformation should happen in your array input directive alone).

Again as I mentioned earlier these are just guidelines. If they're not helpful to you, you can ignore them.

@RFSH
Copy link
Member

RFSH commented Feb 12, 2019

A minimal and temporary solution for having arrays in recordedit is treating them the same way as json columns. Achieving this with the current implementation is simple. We just have to make sure we're mapping array columns to json inputs (Here and here).

If we just do this simple change, then we can at least support arrays. Although we have to rely on ermrest to do the validation of values. The following is the error message that we currently show when a user tries to submit ["test", 1] for an int4[] column:

Error 400 Bad Request The request is malformed. Bad JSON array input. ERROR: invalid input syntax for integer: "test"

@hongsudt Is this message clear enough? Should we just do this simple change? or should we spend more time to have a proper value validation here? If we are going to implement the complete support for arrays, the extra code that we have to add for validation now won't be useful.

@hongsudt
Copy link
Contributor Author

hongsudt commented Feb 13, 2019

We should this get this minimal change in place, so that Facebase can start using it (with training on a small group of user).

However, this error message is very hard to understand.. And if there are multiple array columns, there is no way for me to even know where to address the issue. So I think we should do simple client-side validation if simple or just wait and put the GUI in place for better UX.

@RFSH
Copy link
Member

RFSH commented Feb 23, 2019

The changes for a simple array support has been merged into master. Chaise is going to provide a textarea for array columns which is similar to what we provide for json columns. You have to write the literal string value of the array ( ["value1", "value2"] with brackets and double quotes) in the textarea. We’ve added a customized placeholder for these inputs to help the users. It also has its own special validators to validate the array structure and each individual values in the array.

@jrchudy jrchudy added discussion required requires a discussion before moving forward and removed help wanted Intended for internal use for student workers labels Apr 12, 2022
@RFSH RFSH added help wanted Intended for internal use for student workers and removed discussion required requires a discussion before moving forward labels Jun 22, 2023
@ladukaraniket
Copy link
Contributor

ladukaraniket commented Jun 27, 2023

@hongsudt, @jrchudy and @RFSH ,
I have explored the following 3 approaches to implement the list -

Using the library react-beautiful-dnd [published by Atlassian]

  • It is a lightweight library which allows us to implement drag and drop functionality quite easily.
  • The amount of code to be written is minimal and easy to read and understand.
  • Can support up to 10,000 items @ 60fps with virtual lists and comes with visually appealing animations
  • easy to detect ordering of items in terms of index. Easy to get the output in a required format.
  • dependencies - @babel/runtime, css-box-model, memoize-one, raf-schd, react-redux, redux, use-memo-one
  • Entire list of dependencies
  • Atlassian does not have any further plans to add more features but will continue to make critical security fixes since a lot of Atlassian products depend on this for DND functionality.

react-b-dnd


Using javascript

  • very lightweight and fast.
  • custom logic required to detect row position and rearrange the item once it is dragged.
  • no animations. animations will have to be added if required.
  • All the edge cases will have to be checked and covered to mitigate unexpected behavior.

js


Using the library react-draggable [is a part of react-grid-layout]

  • lightweight as compared to react-beautiful-dnd.
  • makes items draggable anywhere across the screen.
  • the behavior is similar to moving widgets or windows on a screen.
  • custom logic required to restrict the movement of items within a set area and also prevent items from overlapping.
  • custom logic required to detect row position and rearrange the item once it is dragged.
  • dependencies - clsx, prop-types
  • Entire list of dependencies

react-draggable

@carlkesselman
Copy link

carlkesselman commented Jun 27, 2023 via email

@hongsudt
Copy link
Contributor Author

Yes.. This is a very good idea. Then we can personalize the facet display as well..

@carlkesselman
Copy link

actually, and columns!!!

@ladukaraniket
Copy link
Contributor

@hongsudt @RFSH and @jrchudy ,

I've tried a bunch of options as discussed. I've attached all the possible combinations of icons that could be used for -

  • move icon (signifying that the row is draggable) [2 icons]
  • add row icon [2 icons]
  • delete row icon [4 icons]

Text, Int and Float fields

array-text-intint-float-vtext-v


Boolean field

boolean-wvboolean-v


Date Field

Layout 1

date-t1-wvdate-t1-v

Layout 2

date-t2-wvdate-t2-v


Timestamp Field

    Layout 1             |             Layout 2

datetime t1 comdatetime t2 com


Increased Icon size

Screen Shot 2023-06-30 at 19 31 33Screen Shot 2023-06-30 at 19 31 50

firefox_YDHG6xNrh6firefox_dEayZhkcoU

imagefirefox_WV8CPmxbCf

@ladukaraniket
Copy link
Contributor

ladukaraniket commented Jul 7, 2023

Hey @jrchudy and @RFSH , I've updated the spacing between the + and - buttons as discussed yesterday.
I've attached the before and after snapshots for reference. Do let me know if this looks good.

Large width

image

Small width

Previous

image

New

Option 1
image
Option 2
image

@jrchudy
Copy link
Member

jrchudy commented Jul 20, 2023

From further discussion we decided the UI didn't have a way to "communicate" when a set was empty or allowed to be empty. This is the new proposed UI we will go with:
image

The changes from what we had previously discussed include:

  • remove the + button from each array row
  • add an input at the bottom with Add button for adding more array elements
    • new values are appended to the end of the current list
    • add is disabled until value is valid
  • change - to a trash can
    • disable this button when there is only 1 array element and the column is not nullable
  • remove the border and shadow box
  • each array element is editable but cannot be empty
    • add validation to each array element input to validate it's not empty
  • color of grip icon should be a lighter grey

@ladukaraniket
Copy link
Contributor

ladukaraniket commented Aug 2, 2023

Updated the UI as per discussion -

  • Made the font size same for list items and add-new input.
  • Moved delete button to the right for Timestamp type.
  • Center aligned Add button for Timestamp type with single row height.
  • Fixed overall alignment of items.
  • Added an onhover shadow for list items
  • Timestamp date and time fields do not have whitespace between then now.

lWWscLrimp

@ladukaraniket
Copy link
Contributor

ladukaraniket commented Nov 8, 2023

As per commit aeb6c199b05a031c33921f74a98c2613577d80da ArrayField works as expected functionally.

It is unusable in the current state as it suffers from severe performance issues due to the way the overall form implementation is structured and needs work to optimize it.

Refactoring the ArrayField component to use the useFieldArray hook from the react-hook-form and memoizing ArrayField and react-beautiful-dnd components improves the performance 10 folds.

Following a discussion with @RFSH and @jrchudy , the following possible ways to improve the form performance were identified

  • Replace general watch with specific useWatch in the following components:

    • select all
    • array field
    • timestamp
    • foreign key
    • iframe
  • Memoize inputSwitch and other components

  • Replace reset with resetField in form-container

  • use useFieldArray for the inputs that are a collection of inputs

    • array field
    • timestamp
    • foreign key
    • iframe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Intended for internal use for student workers recordedit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants