-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add support for relations #2
Conversation
enable delete,rename,add
Wow, great work! That's a very useful addition to the widget. |
Hi Stefan, Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Fabian,
So sorry to have kept you waiting, have had a lot on my plate recently.
I've finally had a chance now to look over it and also test it locally in my Figma instance. Great job so far and thanks again for contributing! I definitely see this being very useful.
It works as advertised, toggling related properties on/off works really well and is intuitive, and I also especially love the ability to jump to the related widget by clicking on the related type.
There are a couple of things I think we would need to work through before releasing it to others.
Some minor things I've commented directly on the files in the PR.
In addition to those:
- From a design standpoint, while it's a great start, it doesn't completely fit the look and feel of the rest of the widget just yet. However I think there aren't drastic changes necessary to achieve this, just a few minor improvements:
- I think we should consider removing the background for relations and make them more consistent with the rest of the properites, so that they fit in better, and moving down the "show details" toggle so there's no interruption for the eye when scanning the list.
- When adding a new relation, we should probably add an instruction or something when the user is supposed to select to which other entity the relation should be. It took me a minute to get. A short hint when the picker is open could clear that up.
- The "add new relation" panel should probably appear at the bottom of all existing relations. It currently shows up at the very top, which is a bit odd since the the new relation, once added, will be added at the bottom, consistent with how other properties appear when clicking the Plus button.
- The empty state has gotten a bit large. I think we could omit the relation-specific empty state since the instructions are pretty much included in the first empty state.
- When there are no relations, we should hide the divider.
- Here's a quick mockup of how it could look:
-
While testing, I have noticed this behaviour where adding a property to one entity somehow incorrectly syncs to the different related entities. Here's a screencast:
https://user-images.githubusercontent.com/5149339/194778585-bda118ee-c481-46fa-ab5a-b49357a1ff04.mp4
I think this happens when a user duplicates a widget instance to create a new one. While it's a fully new instance, it still seems to be linked to the relation, so any changes to the duplicate are also incorrectly applied to relations of the original. Can you reproduce the issue? -
Also a small thing, but it looks to me like some of the formatting and indentation has become inconsistent. A quick reformat of all files using Prettier should do the trick (there's a .prettierrc in the root dir so it should automatically take the right config).
Thanks again for your effort! I hope I articulated myself clearly enough. Let me know if you have any questions or differing points of view.
} | ||
}) | ||
|
||
const addPropertyToRelatedEntities = (propertyId , propertyToAdd) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arguments propertyId
and propertyToAdd
should be typed instead of implicitly any.
} | ||
|
||
const updatePropertyinRelations = (propertyId: string, property: Property) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you uppercase the "i" in the method name?
}) | ||
|
||
const addPropertyToRelatedEntities = (propertyId , propertyToAdd) => { | ||
let relatedWidgets = [...getRelatedWidgets()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is initialised with let
, but never reassigned as far as I can tell.
availableEntities: relatedWidget.widgetSyncedState["availableEntities"], | ||
typeToAdd: relatedWidget.widgetSyncedState["typeToAdd"], | ||
showRelatedProperties: relatedWidget.widgetSyncedState["showRelatedProperties"] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should consider extracting a function to return this syncedStates
object from a relatedWidget
, as this is repeated multiple times in this file and a bit verbose. This would make it easier and less error-prone down the road if we add additional states to an entity.
} | ||
|
||
const getWidgets = () => { | ||
let collectedWidgets = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is initialised as an empty erray, it is implictly any[]
, which then also becomes the return-type of the function, leading to a lot of any
across all use sites. We should either explicitly denote the type WidgetNode[]
here.
|
||
relatedWidgets.forEach(relatedWidget => { | ||
const state = relatedWidget.widgetSyncedState | ||
let relations: SyncedMap<Relation> = state["relations"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me like relations
is never reassigned, so we can use const
instead.
const title = props.entity["title"] | ||
|
||
return ( | ||
<AutoLayout direction={"horizontal"} verticalAlignItems="center" spacing={4}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous curly braces around "horizontal"
} | ||
|
||
interface EntityRowProps { | ||
entity: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, we should create an interface for Entity
, so we can get better type-safety here.
@@ -0,0 +1,14 @@ | |||
interface SelectEntityProps { | |||
availableEntities: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in EntityRow.tsx
title: string | ||
relatedEntity: string | ||
relatedTitle: string | ||
relatedProperties: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this Property[]
? Or (Property & { propertyId: string })[]
?
Since both seem to be used, we should probably make it clear here which one it is. (Maybe also create a type for the Property type that includes its id)
Add a new property type "relation" which enables the user to connect one entity with another.
Relations and its related properties will be listed separately. The user can toggle the visibility of the related properties.
There is a switch to enable/disable each related property. If disabled, the opacity of the property is reduced, to indicate it is not used in the entity.
If a property gets added, updated, or deleted in the parent entity, the relation gets updated. In the case of "add property", this property will be disabled in the related entity.
Each relation is linked to its parent. If clicked on, the viewport adjusts to focus on the parent.
Image Select Relation:
![select-relation](https://user-images.githubusercontent.com/84379209/190143688-1b777adb-4a35-41e6-9ce7-4140d98718d0.png)
Image Relation:
![relations](https://user-images.githubusercontent.com/84379209/190143681-36958ce6-dcda-4dbe-93b3-65eb3dd837cf.png)
Further plans from my side:
Let me know what you think.