Edit object references #175

itsjgf · 2017-09-28T19:51:04Z

Closes #168 and closes #37.

Summary

You can edit an object reference by double clicking on it or right click + “Update reference”.
When the modal is open, a list with the current possible references appears and the “Ok” button displays “Set to null”.
Once you select a new object, the button will change from ‘Set to null’ to ‘Set’.

Additional changes which are not related to this PR:

Table: Enable row selection on click and editing on double click.
Row highlighting: Add missing columnIndex.
Edit cell: Add missing autofocus.

bigfish24 · 2017-09-29T07:34:13Z

Testing this out it looks pretty good, but the UI/UX is difficult due to the selection behavior.

It should be that a single-click selects a row and a double-click goes into edit for a cell.

When I right click on an object reference, I would expect the row to be highlighted:

The row is not highlighted so it is hard to see which object I right clicked on. Also, minor, but I think "Update Reference" should be first.

Similarly, on the modal to select a reference:

I think you should select the row, then click have the blue button change to "Set" vs. "Set to null" to confirm the selection. @kraenhansen thoughts?

itsjgf · 2017-09-29T08:21:57Z

Totally agree. I was trying to avoid changing the current cell implementation which is not highlighting rows nor managing click/double click 'properly'.

Working on it 😊.

kraenhansen

Minor changes required - and I did a small commit on the branch as well.

kraenhansen · 2017-09-29T08:53:11Z

src/ui/realm-browser/Content.tsx

+            style={{ userSelect: 'none', cursor: 'pointer' }}
+            onClick={e => {
+              e.stopPropagation();
+              e.preventDefault();


Is this needed? What is the default behaviour of clicking a div?

kraenhansen · 2017-09-29T08:53:30Z

src/ui/realm-browser/Content.tsx

-  onCellChange: (object: any, propertyName: string, value: string) => void;
-  onListCellClick: (
+  onCellChange?: (object: any, propertyName: string, value: string) => void;
+  onListCellClick?: (


What is the reasoning behind making these optional?

If we want to be able to reuse the table (e.g to select a row), there are some props that should be optional, like that one.

kraenhansen · 2017-09-29T08:55:43Z

src/ui/realm-browser/RealmBrowser.scss

@@ -121,6 +121,14 @@ $realm-browser-header-icon-space: 24px;
    flex-direction: column;
  }

+  &__SelectObject{


&__SelectObject { <- missing a space :)

kraenhansen · 2017-09-29T08:56:40Z

src/ui/realm-browser/RealmBrowser.scss

+    display: flex;
+    flex-direction: column;
+    padding:0;
+  }


Missing a space .. here as well. And the properties are not ordered, are you SCSS linting?

kraenhansen · 2017-09-29T09:02:21Z

src/ui/realm-browser/Content.tsx

    object: any,
    property: Realm.ObjectSchemaProperty,
    value: any,
  ) => void;
  onColumnWidthChanged: (index: number, width: number) => void;
  schema: Realm.ObjectSchema | null;
-  rowToHighlight: number | null;
+  rowToHighlight?: number | null;


I don't like that this can now both be undefined (because it's optional) and null because of its type.
I think it should just be

rowToHighlight?: number

or just left as

rowToHighlight: number | null

Otherwise we have two ways of checking if no row is selected (if we're not using Number.isInteger(...))

I believe that null was there cause the old linter was throwing some errors. Totally true, we have to re-check the types.

kraenhansen · 2017-09-29T09:10:49Z

src/ui/realm-browser/RealmBrowser.scss

+    min-height: 300px;
+    padding: 0;
+  }
+


I added some spaces and sorted the lines - are you running the SASS linter?

I wouldn’t worry too much about this. We were going to propose yesterday to use prettier + stylelinter so we don’t have to fix this stuff manually, but the new features seemed more important.
Also, we could add css-modules in order to avoid long class names (e.g RealmBrowser__Content__Cell could be just cell or Cell).

I can fix them right now if that's a priority 😛

kraenhansen · 2017-09-29T09:14:46Z

src/ui/realm-browser/SelectObject.tsx

+    <ModalFooter>
+      {optional && (
+        <Button color="primary" onClick={() => updateReference(null)}>
+          Set to null


I don't think this should be the primary action - if we had a way to select the row in the content table - I would also expect there to be a primary button to "Select" the object of which row was selected, instead for this happening when simply clicking the row.

kraenhansen · 2017-09-29T09:16:10Z

src/ui/realm-browser/SelectObject.tsx

+            schema={schema}
+            data={data}
+            onRowClick={updateReference}
+            onListCellClick={updateReference}


I know this PR didn't add the onListCellClick but this name confuses me - and why would I need to define this property here, wouldn't the click bubble to the onRowClick callback anyway?

I didn’t add that prop but that’s the one handling the click event for “complex types (Objects/Lists), the simple types don’t have one (they got one internally assigned tho!).

I think that we should have only one onClick event for the cells (from onListCellClick to onCellClick and pass it to simples types too) and add another one for onDoubleClick (onCellDoubleClick). And get rid of onRowClick 😅.

What do you think about it?

…ouble click, improve 'SelectObject' and 'StringCell'

kraenhansen

For the sake of making progress over perfection - let's get this merged.

bigfish24 · 2017-09-30T16:56:30Z

It’s not clear if Commite is around on the weekend so we should merge if we are happy.

itsjgf added 3 commits September 28, 2017 21:37

feat(realm-browser): add SelectObject

61ed323

feat(realm-browser-content): add onRowClick handler

957133f

feat(realm-browser): allow object references to be edited

6e97dbe

itsjgf changed the title ~~(WIP) Edit object references~~ Edit object references Sep 28, 2017

itsjgf requested review from bigfish24, kraenhansen and IvanCoronado September 28, 2017 21:18

Adding spaces and sorting properties

0b0e875

kraenhansen requested changes Sep 29, 2017

View reviewed changes

itsjgf added 3 commits September 30, 2017 01:06

feat(realm-browser): enable select rows on single click and edit on d…

5b9fbd3

…ouble click, improve 'SelectObject' and 'StringCell'

feat(realm-browser): add missing 'columnToHighlight' prop

1c2138f

feat(realm-browser): improve typings and context-menu order

8f6df1f

kraenhansen approved these changes Sep 30, 2017

View reviewed changes

itsjgf merged commit bc3858a into master Sep 30, 2017

itsjgf deleted the jg/edit-object-reference branch September 30, 2017 18:13

github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit object references #175

Edit object references #175

itsjgf commented Sep 28, 2017 •

edited

Loading

bigfish24 commented Sep 29, 2017 •

edited

Loading

itsjgf commented Sep 29, 2017 •

edited

Loading

kraenhansen left a comment

kraenhansen Sep 29, 2017

kraenhansen Sep 29, 2017

itsjgf Sep 29, 2017 •

edited

Loading

kraenhansen Sep 29, 2017

kraenhansen Sep 29, 2017

kraenhansen Sep 29, 2017

itsjgf Sep 29, 2017

kraenhansen Sep 29, 2017

itsjgf Sep 29, 2017

kraenhansen Sep 29, 2017

kraenhansen Sep 29, 2017

itsjgf Sep 29, 2017 •

edited

Loading

kraenhansen left a comment

bigfish24 commented Sep 30, 2017

Edit object references #175

Edit object references #175

Conversation

itsjgf commented Sep 28, 2017 • edited Loading

bigfish24 commented Sep 29, 2017 • edited Loading

itsjgf commented Sep 29, 2017 • edited Loading

kraenhansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsjgf Sep 29, 2017 • edited Loading

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

itsjgf Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

kraenhansen left a comment

Choose a reason for hiding this comment

bigfish24 commented Sep 30, 2017

itsjgf commented Sep 28, 2017 •

edited

Loading

bigfish24 commented Sep 29, 2017 •

edited

Loading

itsjgf commented Sep 29, 2017 •

edited

Loading

itsjgf Sep 29, 2017 •

edited

Loading

itsjgf Sep 29, 2017 •

edited

Loading