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

💥Dropped link to add node and automatically connect its link via components panel #121

Merged
merged 27 commits into from
Mar 24, 2022

Conversation

AdrySky
Copy link
Contributor

@AdrySky AdrySky commented Mar 17, 2022

Description

This will enable to add node via context menu. It's also possible to automatically connect its link by dropping a link to canvas.

The components panel will only display what component it can linked but not smart enough to know 'Can it connect?' as there is no type check when the link is connecting.

There are 3 condition to display 3 different components list which are:

  1. Right-clicking canvas will show all component that can be added.
  2. Dropping link from ▶ either inPort or Outport will show all but General Components. (Can be added and linked)
  3. Dropping link from any parameter inPort(beside ▶) will only display General Components. (Can be added and linked)

Take note:
Dropping link from any parameter OutPort(beside ▶) will not display anything as it's hard to capture what it can link to.

As this component panel can be activated by right-clicking, it make the previous Jupyterlab's context menu obsolete. With that, new custom context menu was created with fewer command mostly for nodes. It can be activated by ctrl+left-click.

Also, refactored and removed all the general components except literal and hyperparameter.

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. Try right-clicking canvas.
    1. A panel showing components will opened. (The previous context menu was removed)
    2. Choose any node to add it.
  2. Try dropping link based on the condition mentioned above and see it's the same as mentioned
    1. Link from ▶ either from inPort or OutPort.
    2. Link from any parameter inPort(beside ▶).
  3. Also, when choosing the node, does the link automatically connect?
  4. When component panel is displayed, try using the search bar and click the display node.
    1. Try dropping different link and search again.
    2. It should only show the allowable components.
  5. Try opening the new context menu by ctrl+left-click.
    1. Try each option and does it work as expected.
  6. Try ctrl+left-click any node excpet general components at the components panel
    1. It'll open its python script

Tested on?

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

Notes

Currently, there is no type check when connecting the link. Also, feels like some of the files in /src/component folder need to move to different folder.

@AdrySky AdrySky added the enhancement New feature or request label Mar 17, 2022
@AdrySky AdrySky requested a review from MFA-X-AI March 17, 2022 07:33
@AdrySky AdrySky self-assigned this Mar 17, 2022
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.

Thanks for the pull request!

This feature for sure be able to speed up developing xircuit workflows. From first try, looks like the first wheel is still a bit buggy when any component from the component panel is created. To reproduce:

  1. create component
  2. drag arrow -> component panel will pop up
  3. search component -> drop -> component will be created at 0, 0
    I've also noted the log:
Uncaught TypeError: Cannot read properties of undefined (reading 'getOptions')

@AdrySky
Copy link
Contributor Author

AdrySky commented Mar 22, 2022

Thanks Fahreza for the review. It seems the problem occur when we built the wheel. It doesn't built the frontend when packaging the xircuits.

So to deploy simultaneously the frontend and the backend, the frontend NPM package needs to be built and inserted in the Python package. Ref

The workaround is I just add the labextension so that it can build the frontend when packaging.

@AdrySky AdrySky requested a review from MFA-X-AI March 22, 2022 06:32
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.

Awesome work, it feels much natural to use now!

There's 2 things that I'd like to comment on.

  1. The spawn location for the component bar. In smaller screens or if you drag it close to the edge, it'll clip left by a large margin.
  2. The component spawn location for right click. It goes to a location which I'm not very sure where it's based.

pop-up location drop location

I think both can be solved if you can both spawn the menu and component at where the pointer is. I think the tricky part is how to translate the zoom ins and out coordinates, but it shouldn't drift too far out even if you don't have a handler for it. That, or hopefully RD has xy absolute coordinates.

@AdrySky
Copy link
Contributor Author

AdrySky commented Mar 24, 2022

Thanks for the feedback. Fixed both of the issues. Another thing I notice while fixing this is if you open the panel near to right side of screen, it'll go off screen. What do you think about this?

Edit: Another things is the labextension folder. I don't think it's a good idea to have it in master. Because it'll generate many different files with each build so we've to push that change. I think I should remove it but need to make some change in github action to build the frontend when packaging.
The change needed for github action is do pip install -e . before:

name: Build Wheel
      run: |
        python -m pip install --upgrade build &&
        python -m build

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.

Awesome work, everything works smoothly now. Merged and thanks for the PR!

@MFA-X-AI MFA-X-AI merged commit 781321e into master Mar 24, 2022
@MFA-X-AI MFA-X-AI deleted the adry/components-panel branch March 24, 2022 09:29
@AdrySky AdrySky mentioned this pull request Jun 7, 2022
15 tasks
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.

2 participants