Skip to content
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

fix: add attribute hover style is not applied #66

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

oltionchampari
Copy link
Contributor

@oltionchampari oltionchampari commented Oct 3, 2023

Closes https://github.com/Caleydo/cohort/issues/718

Developer Checklist (Definition of Done)

Issue

  • All acceptance criteria from the issue are met
  • Tested in latest Chrome/Firefox

UI/UX/Vis

  • Requires UI/UX/Vis review
    • Reviewer(s) are notified (tag assignees)
    • Review has occurred (link to notes)
    • Feedback is included in this PR
    • Reviewer(s) approve of concept and design

Code

  • Branch is up-to-date with the branch to be merged with, i.e., develop
  • Code is cleaned up and formatted
  • Unit tests are written (frontend/backend if applicable)
  • Integration tests are written (if applicable)

PR

  • Descriptive title for this pull request is provided (will be used for release notes later)
  • Reviewer and assignees are defined
  • Add type label (e.g., bug, feature) to this pull request
  • Add release label (e.g., release: minor) to this PR following semver
  • The PR is connected to the corresponding issue (via Closes #...)
  • Summary of changes is written

Summary of changes

  • The error was a result of the event.currentTarget being null by the time the setTimeout is executed
  • This fixes also the hover background on the gene when the gene datatype is hovered

Screenshots

before

gr-before

after

gr-after

Additional notes for the reviewer(s)

Thanks for creating this pull request 🤗

@oltionchampari oltionchampari added release: minor PR merge results in a new minor version type: bug Something isn't working labels Oct 3, 2023
@oltionchampari oltionchampari self-assigned this Oct 3, 2023
@oltionchampari oltionchampari changed the title fix: add attribute hover is not applied fix: add attribute hover style is not applied Oct 3, 2023
setTimeout(() => {
// update detail after timeout time global and current optionId is equal
if (d.optionId === this._geneHoverOptionId) {
this._mouseOverHandler(d, event.currentTarget);
this._mouseOverHandler(d, currentTarget);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be necessary, but you can also use it on line 311.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 311 only handles the case for non-genes. Ollie fixed the gene case here and the event might not be available after the timeout is executed. So I agree with the fix he proposed.

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is fixed with your changes.

setTimeout(() => {
// update detail after timeout time global and current optionId is equal
if (d.optionId === this._geneHoverOptionId) {
this._mouseOverHandler(d, event.currentTarget);
this._mouseOverHandler(d, currentTarget);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 311 only handles the case for non-genes. Ollie fixed the gene case here and the event might not be available after the timeout is executed. So I agree with the fix he proposed.

@thinkh thinkh merged commit d74529b into develop Oct 13, 2023
2 checks passed
@thinkh thinkh deleted the och/718-fix-add-attribute-hover branch October 13, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: minor PR merge results in a new minor version type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants