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

✨Add Component's Description Tooltip #151

Merged
merged 11 commits into from
Apr 28, 2022

Conversation

AdrySky
Copy link
Contributor

@AdrySky AdrySky commented Apr 15, 2022

Description

This will display tooltip of component's description. It get the information from its script top level docstring(After class, before port information).

Component Description Tooltip

Pull Request Type

  • Xircuits Core (Jupyterlab Related changes)
  • Xircuits Canvas (Custom RD Related changes)
  • Xircuits Component Library
  • Testing Automation
  • Documentation
  • Others (Please Specify)

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Tests

  1. Make sure it's able to do the same as shown in gif

Tested on?

  • Windows
  • Linux Ubuntu
  • Centos
  • Mac
  • Others (State here -> xxx )

@AdrySky AdrySky added the enhancement New feature or request label Apr 15, 2022
@AdrySky AdrySky self-assigned this Apr 15, 2022
Copy link
Member

@mansouralawi mansouralawi left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks for the PR

Feature: display tooltip of component's description. It gets the information from its script top level docstring(After class, before port information). works fine

After a few tests on this PR, I would like to point to some expected bugs:

  1. The description box persists on display after switching from the canvas to another tab

issue_1

  1. The description box does not follow the component when zooming the canvas in/out or dragging the whole canvas

issue_2

  1. The description box persists in front of the context menu

issue_3

  1. Clicking on the output port removes the icon to open the discerption box. Need to reload node to make it appear again

issue_4

  1. The description box can be activated on multiple components. However, one description box will serve them all. Here I suggest two possible fixes:
    -If the description box is activated on one component, it deactivates the description boxes on all others.
  • If the description box is activated on multiple components, each component gets a standalone description box.

issue_5

Copy link
Member

@MFA-X-AI MFA-X-AI left a comment

Choose a reason for hiding this comment

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

That was a great review, thanks @mansouralawi. The information panel does share some similarities with the error bubble, so I'll throw in a few comments.

  1. The description box persists on display after switching from the canvas to another tab
  2. The description box does not follow the component when zooming the canvas in/out or dragging the whole canvas
  3. The description box persists in front of the context menu

This also happens with the error bubble, the difference is that the bubble is dismissed after 1-2 seconds so it's not so noticeable. A quick fix would be to apply the same for the information panel, but long term we definitely want to dismiss it when changing tabs.

  1. Clicking on the output port removes the icon to open the discerption box. Need to reload node to make it appear again

This one was interesting. When you click on an outPort, it's treated as a drag and drop action, hence creating a link to the same port which throws out the error bubble. Definitely should look into it being rerendered though, possibly the state of the component.

  1. The description box can be activated on multiple components. However, one description box will serve them all. Here I suggest two possible fixes:
    -If the description box is activated on one component, it deactivates the description boxes on all others.
  • If the description box is activated on multiple components, each component gets a standalone description box.

While unintentional, I actually like the way it's being handled now. Users would typically only have attention towards one component description at a time, but if they have multiple info blocks active, it would have the moving animation (which indicates it was already active in another component). But yes, 1 info box at a time would reduce the visual cluster I think.

@AdrySky
Copy link
Contributor Author

AdrySky commented Apr 21, 2022

Thanks @mansouralawi & @MFA-X-AI for the feedback. I think it was a bit rushed of me making this PR as there are lot of bugs laying around.

I could fix some issue if kept using react-portal-tooltip but not all. So I use different ones called react-tooltip to solve all bugs mentioned.
Also, I updated the style of error tooltip.

Copy link
Member

@mansouralawi mansouralawi left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes, most of the pervious issues are solved, here a list of some of thee bugs I found.

  1. Description box pop-out away from the component.
    issue_1

  2. Connection warning appears within the component.

issue_2

  1. Overall the canvas is buggy when dragging the components or connections Major Issue , Better to be tested again on another device.

@AdrySky
Copy link
Contributor Author

AdrySky commented Apr 26, 2022

Thanks @mansouralawi for the review. So I fixed the latest bug issues. Though, not sure about the last issue mentioned,

Overall the canvas is buggy when dragging the components or connections Major Issue

Not sure what causing it. Let me know if it happen again. Thanks again.

@AdrySky AdrySky requested a review from mansouralawi April 26, 2022 01:59
Copy link
Member

@mansouralawi mansouralawi left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes and sorry for the delay.

Most of the features are working fine.

  • Just the description box and warning are experiencing some shift to the right side.
    issue-1

@AdrySky
Copy link
Contributor Author

AdrySky commented Apr 28, 2022

Thanks for noticing that.
Bug fixed. Need to include the width of the left sidebar when overriding the tooltip's position.
Edit: Forgot to do the same for error tooltip

@mansouralawi
Copy link
Member

Great work, feature works as expected.

@mansouralawi mansouralawi merged commit 1060be5 into master Apr 28, 2022
@mansouralawi mansouralawi deleted the adry/component-docstring-tooltip branch April 28, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants