-
Notifications
You must be signed in to change notification settings - Fork 63
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
First draft #7
First draft #7
Conversation
classes = Unicode().tag(sync=True) | ||
|
||
data = Dict(trait=Instance(Data)).tag(sync=True) | ||
position = Dict(trait=Instance(Position)).tag(sync=True) |
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.
I am scared this would not scale. You'll end up creating too many widgets. Those two can stay traits I guess.
Creating too many widgets is also not good, you create too many communication with the frontend.
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.
hum! haha
good thing I posted it here before doing more of the same, I was just asking Wolf something.
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.
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.
I guess one rule you can follow is: if you don't need JavaScript logic specific to it, make it a trait, not a widget class.
counter += 1 | ||
if node_id == n['data']['id']: | ||
n['data']['name'] = attrs | ||
graph = Dict(trait=Instance(Graph)).tag(sync=True) |
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.
I think Instance(Graph) is more appropriate (without Dict wrapper).
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.
Why is there a Graph
and a Cytoscape
widget? Can we not get rid of the later?
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 a Cytoscape widget is more complex than just a graph, a graph is just an attribute of a Cytoscape thing.
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.
What is a Cytoscape widget?
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.
It contains a graph and information like the kind of layout you should be using, the style and other properties like the zoom level of your object, the position, etc.
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.
Couldn't those be graph attributes?
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.
Or do you expect the same graph to be displayed multiple times on the page with different layouts maybe?
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.
Hum... it feels more organized to me to have a Graph class.
But if you think it's a good trade off to have fewer Widget classes, than I suppose it'd be fine. I don't think having multiple layouts of the same graph is a common use case, also, if people wanna do that they can make copies.
elements: [], | ||
zoom: 0, | ||
rendered_position: {}, | ||
graph: {}, |
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.
Change to null
if you change to Instance
(or was it undefined
? @martinRenou
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.
None
in Python is translated to null
, so null
might be better
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.
If allow_none is False, we have generqlly defaulted to undefined for subwidgets.
@@ -55,8 +49,8 @@ class CytoscapeModel extends DOMWidgetModel { | |||
static model_name = 'CytoscapeModel'; |
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 serializers (which is hidden above) need to deserialize the graph
widget.
This should look something like this:
static serializers: ISerializers = {
graph: { deserialize: widgets.unpack_models },
...DOMWidgetModel.serializers
}
Let me check this further (you need to list each attribute that's a widget that needs serialization like this).
Thanks for the help ppl. This was just a draft that won't be merged. |
The value used with the `-e` option to `pip install` is `.[test]`. The `[` character has [special meaning](https://tldp.org/LDP/abs/html/special-chars.html#LEFTBRACKET) in many popular unix shells, so must be quoted (or escaped) to get the literal character needed for `pip install` to interpret the value inside the square brackets as a local directory path. A demonstration using `zsh`: ``` % pip install -e .[test] zsh: no matches found: .[test] ``` Quoting `.[test]` yields the expected result: ``` % pip install -e ".[test]" Looking in indexes: ... ``` It seems the pypa documentation has the same problem - see [`pip install` example cytoscape#7](https://pip.pypa.io/en/stable/cli/pip_install/#example) . Those examples are all prefixed with `python -m pip`, however the same shell quoting rules still apply. I'll file a similar PR over there.
No description provided.