-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feature/display label defaults #443
Conversation
Using a function to create labels on the fly adds a compositional feature that we no longer need, since display label is always displayVariable/first prop/id. I'm wondering if this would make sense as an addition to the network selectors instead - that way we could remove the label function entirely. |
This is certainly possible, to move away from the function approach. Although in discussing this with @wwqrd it raises a few questions that I'm still mulling over. For example, we could modify the nodes in the reducer, or in the selector. Doing this in the reducer would have certain implications -- for example, downloading data would then have our "label" field. Is that a plus or a minus? Complicating this idea is that we have external data which would need a similar massaging to map our "label" on to it. We could do this either in the reducer (when parsing the protocol) or the selector (when accessing the node providers). But async external data is in the plans and I'm not entirely sure what implications that might have. And finally, we add a label as a prop and then all the node attributes using the spread operator. This means if a node already has a On a slightly related note, a similar thing happens if users have a "color" field on a given node. I think that is something we intended, but there are other props we use that could similarly be clobbered by existing fields in the data (e.g. "selected", "meta", "allowDrag", etc). This prop clobbering is perhaps related to our uid discussion, though likely should be a separate issue. |
Good to see how much thought you've put into this @rebeccamadsen! Lots of excellent questions.
Here was the thought process I went through with this:
To summarize, I think the calculated value of a display label should stay out of the exported data because it is either a combination of other properties, or can be stored in a different way if the user wishes.
External data is indeed complicated, and we haven't fully thought through it yet. However, I think the current logic still stands. So basically (see my note below for why the order of the following is altered), (1) look for a label property with a set name (like
I think you've brought up an excellent point here, which helps to refine the idea a bit. The way I see it now is that the display variable setting in the node registry should function like an override. That is, by default we will look for a This will mean that wherever a label property is present it will be used, an we will let users decide if they wish to modify this behavior manually. As for what this property is called, I've used label above because I think it is obvious. If you think a more specific property would be better then go for it. I don't think label is a variable name that researchers will use unintentionally, so I think it is fairly appropriate. |
The algorithm you describe is what is implemented here -- 'label' from the data is preferred, then displayVariable, then first property on a node, then node id (though (3) first useful looking property is questionable -- right now it is just taking the first property, but maybe it should take the first property that isn't 'id' or 'uid'. That could still be something weird like 'age', but I guess that's why it is the third solution instead of the first or second...). I think the remaining debate is whether the display label should be a function in the selector, that NodeList calls for each node (how it is implemented here so far), or if it should be on the selectors that get network nodes, and on selectors for node providers, and the future (selectors?) for async external data (so long as saving the network to a file doesn't use those same selectors). And that perhaps we should still consider documenting somewhere what props we are using that nodes could inadvertently override. Some of the props we throw on a node I could see a plausible reason for allowing users to override, like |
Yep. But this wasn't the original intention. We have been very clever in updating our spec to match our behavior 😉
I think the idea was to have something like the first text variable. I'm not sure how feasible this is. Perhaps if a variable of type text isn't available (either because there just isn't one, or because the data is from an external source and has no variable registry) that can be when we skip to the final fallback of using ID. |
…e/displayLabel-defaults # Conflicts: # src/containers/Interfaces/NameGeneratorAutoComplete.js # src/selectors/interface.js
You brought a up a great point previously about this being related to the UID issue. In that issue I talked about the idea of private properties. I think a low tech solution to implementing that idea and avoiding collisions would be to have our special net canvas properties begin with an underscore ( |
Alright. I updated case 3 to look at the first variable in the registry of type 'text', and added tests. I can add an issue regarding the possible collisions to discuss further. |
Following up from #305... this is great! |
This resolves #311 and (I believe) the last case for #305. It updates node labels so they are no longer hardcoded to display
node.nickname
. Instead, this has a three-fold fallback solution to attempt to use a node's type to lookup thedisplayVariable
for that type in thevariableRegistry
. If that fails, it shows the first field in the node object, and if that fails it showsnode.id
.This should address the situation that nodes of different types can end up in a node list together, and still show something intelligent on a node specific basis.
To verify, experiment with prompts with different node types, giving them identical
additionalAttributes
and adding nodes on each.NodeList
s should show both node types with something intelligent for each label.