Adding some events and handler #225
Conversation
|
@Eng-Elias any news here? |
|
I opened this pull request before about 2 years and asked for review but I didn't get any response, so I left it? @rtviii |
|
Good point! I assumed they are waiting for conflict resolution with mainline. But these on hover etc events would be quite useful to have. |
|
@jjti @guzmanvig any feedback for the samaritan above? |
I am sorry but when I opened the PR, I ensured that there was no conflict then asked for the review. |
|
Likewise, sorry, i just saw this last week. It's a shame the authors missed this back when you contributed. |
No problem, it is a small issue, let me know if you want anything from my side |
|
Hi @Eng-Elias, @rtviii , Apologies for the long silence on this PR — it slipped through the cracks. If you’re still interested, could you please resolve the merge conflicts? Once that’s done, we’ll review it promptly and work on getting it merged. Thanks! |
|
Done, I resolved the conflicts and fixed the tests |
There was a problem hiding this comment.
This is neat! I made some comments, but rejecting mainly because:
- Seems like we are missing the functionality for Primers, Highlights and Translations. What I mean by that is that for example, clicking on a primer, highlight or translation doesn't trigger the onClick event.
- Seems like these events are not supported in the circular view? Or at least they don't seem to work in the demo app.
- Some ESLint errors. Really minor, like commented console.logs or not sorted arguments. You probably need to configure Eslint in your editor so it picks up the rules from
.eslintrc.json
Thanks!
| @@ -1,4 +1,6 @@ | |||
| import * as React from "react"; | |||
| import { Menu as ContextMenu, Item, Separator, Submenu, useContextMenu } from "react-contexify"; | |||
There was a problem hiding this comment.
nit: Separator and Submenu are not being used, remove import.
| import seqparse from "seqparse"; | ||
| import tippy from "tippy.js"; | ||
| import "tippy.js/dist/tippy.css"; | ||
| import { TextSpan } from "typescript"; |
There was a problem hiding this comment.
nit: This import is also not being used, remove import
| Sidebar, | ||
| } from "semantic-ui-react"; | ||
| import seqparse from "seqparse"; | ||
| import tippy from "tippy.js"; |
There was a problem hiding this comment.
Any reason not to use the component for tooltips from Semantic UI (since it's already being used)? https://semantic-ui.com/modules/popup.html#/definition
| }); | ||
| }; | ||
|
|
||
| handleContextMenuAddReverseTranslation = ({ event, props, triggerEvent, data }) => { |
There was a problem hiding this comment.
nit: Could combine handleContextMenuAddForwardTranslation and handleContextMenuAddReverseTranslation into one function with an argument for the direction
| id: CONTEXT_MENU_ID, | ||
| }); | ||
|
|
||
| this.showContextMenu = show; |
There was a problem hiding this comment.
Why creating it on line 120 to then re-assign it to what useContextMenu returns? Is this for the sake of renaming show?
| highlights={[{ start: 0, end: 10 }]} | ||
| name={this.state.name} | ||
| onClick={(element, circular, linear, container) => { | ||
| // console.log({ element, circular, linear, container }); |
There was a problem hiding this comment.
Would be nice to also show the functionality of these other events. Could be as simple as a single tooltip for all that display some info of the event and what was clicked (like the one you did for annotations on hover)
| <strong>Name</strong>: <span>${element.name}</span> | ||
| </div> | ||
| <div> | ||
| <strong>position</strong>: <span>[${element.start}-${element.end}]</span> |
There was a problem hiding this comment.
Nit: Use capital P on position
| </SeqViz> | ||
| )} | ||
| </div> | ||
| <footer> |
There was a problem hiding this comment.
nit: add a small padding/margin on the left
| event: React.MouseEvent<Element, MouseEvent> | ||
| ) => void; | ||
| onDoubleClick?: (element: any, circular: boolean, linear: boolean, container: Element) => void; | ||
| onHover?: (element: any, hover: boolean, view: "LINEAR" | "CIRCULAR", container: Element) => void; |
There was a problem hiding this comment.
Any reason why onHover has a single paramview: "LINEAR" | "CIRCULAR" instead of circular: boolean, linear: boolean params like the others?
| fullSeq: string; | ||
| inputRef: InputRefFunc; | ||
| lastBase: number; | ||
| onClick?: (element: any, circular: boolean, linear: boolean, container: Element) => void; |
There was a problem hiding this comment.
nit: Would it be better to create a single prop typeeventHandlers: EventHandlers to avoid duplicating these types everywhere?
|
I read your comments and I will work on them later because I am a little bit busy these days. |
Thanks @Eng-Elias! Just in cased you missed it, I emailed you (to the address in your Github profile) regarding your use case and other PRs you have open |
Hello @jjti ,
I added some features to SeqViz to increase the ability to control events triggered from elements like bases, translations and annotations where you can pass onClick, onDoubleClick, onContextMenu and onHover, then you can check the type of element and do some action. I added some example how we can use these handlers.
I also added onKeyPress event handler if developer wants to set some custom keyboard shortcuts.
I think onHover event handler will help to display more details about elements.
onContextMenu and onKeyPress will be useful to build SeqViz editor not only a viewer.
Please review my changes and give me your opinion.
I am waiting to hear from you.
Thank you to consider my pull request.
@Eng-Elias