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

[WIP] Adding Tree2D to the UI sub-module #460

Closed
wants to merge 44 commits into from

Conversation

antrikshmisri
Copy link
Member

This PR adds Tree2D UI to the ui sub-module.

@pep8speaks
Copy link

pep8speaks commented Jul 15, 2021

Hello @antrikshmisri! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 152:1: E305 expected 2 blank lines after class or function definition, found 1

Line 3512:80: E501 line too long (84 > 79 characters)

Comment last updated at 2021-08-24 18:39:08 UTC

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #460 (62e47fa) into master (0f97a6d) will increase coverage by 0.07%.
The diff coverage is 89.08%.

❗ Current head 62e47fa differs from pull request most recent head d568836. Consider uploading reports for the commit d568836 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   88.53%   88.60%   +0.07%     
==========================================
  Files          31       31              
  Lines        6575     6968     +393     
  Branches      787      838      +51     
==========================================
+ Hits         5821     6174     +353     
- Misses        535      564      +29     
- Partials      219      230      +11     
Impacted Files Coverage Δ
fury/ui/elements.py 88.63% <88.37%> (+0.09%) ⬆️
fury/ui/tests/test_elements.py 84.52% <91.39%> (+1.29%) ⬆️
fury/ui/containers.py 84.03% <0.00%> (+0.22%) ⬆️

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @antrikshmisri,

I have few questions:

  • is it normal to be able to select (highlight) multiple elements. if yes, can we disable it easily?
  • When you click on the text, you can not select, can you allow the same behavior as when we click outside the text (same comment for the icon)
  • the file test.py should be an additional tutorial so you will need to add an explanation and put it in the right folder

docs/tutorials/02_ui/viz_tree_ui.py Outdated Show resolved Hide resolved
docs/tutorials/02_ui/viz_tree_ui.py Outdated Show resolved Hide resolved
docs/tutorials/02_ui/viz_tree_ui.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
@antrikshmisri
Copy link
Member Author

Hey @skoudoro ,

  • is it normal to be able to select (highlight) multiple elements. if yes, can we disable it easily?

I think it totally depends on the use case, some should allow multiselecting and other shouldn't. Nonetheless, I think this feature should be added. All the changes along with this feature will be added with the latest commit.

@antrikshmisri
Copy link
Member Author

Hey @skoudoro , all the comments have been addressed as well as the multiselect feature has also been added.

@antrikshmisri
Copy link
Member Author

Hey @skoudoro, all the proposed changes have been added. PTAL.

@skoudoro
Copy link
Contributor

skoudoro commented Dec 5, 2022

Hi @antrikshmisri, Before starting to review this PR, Can you fix all the conflict, rebase it and address the small pep8 above?

Thank you

@ganimtron-10
Copy link
Contributor

Hey @antrikshmisri ,
Can you please rebase this PR?

@ganimtron-10
Copy link
Contributor

ganimtron-10 commented Jul 14, 2023

Closing this PR here and continuing at #821 as we got no response from author.

@antrikshmisri
Copy link
Member Author

Sorry @ganimtron-10 for not being active, I am a bit tied up right now. Let me know if you need any context/help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants