-
Notifications
You must be signed in to change notification settings - Fork 2
Allow user to specify hoverinfo #1
Allow user to specify hoverinfo #1
Conversation
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 looks pretty good, just a couple of points before merging.
I think calling the prop hoverPattern
instead of titlePattern
might be a more accurate description, and would make a little more sense in this case.
It seems like the default structName
key is not set; if I leave the prop unassigned in my component, the hovertext is empty - 3
rather than the base name. Setting the prop to '${name}:${num}'
instead does give us the correct hovertext (eg. A - 3
).
And lastly, I think we should explicitly state in the prop description which keys are accepted for this pattern. If it's the following node properties, we should include a list of them in the prop description and possibly output a warning if unsupported keys are passed to the pattern.
self.nodes = [{ 'name': 'P',
'num': 1,
'radius': 3 * Math.sqrt(size),
'rna': self,
'nodeType': 'protein',
'structName': structName,
'elemType': 'p',
'size': size,
'uid': generateUUID() }];
By default, we use the 'empty' structName property. So if custom structName isn't passed, the |
Show warning if prop key not allowed
602ca51
to
47a0437
Compare
@HammadTheOne I've renamed props and added console warning if unsupported keys are passed to the pattern. |
I see, so that way if we have multiple molecule sequences, it will show their structure name's by default on hover. That makes sense. In that case, can we maybe use a ternary operator to set the default hoverTest, checking to see if |
Yes, we can. In this case, we should remove default props from |
Can you please check my function for getting hoverPattern? Is my understanding correct? |
@nickmelnikov82 That looks good to me, the only change I would make is changing |
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.
💃
Tested it out, and the hoverPattern
default works like we described. Fantastic!
@HammadTheOne so, can we merge this PR? |
plotly/dash-bio#519