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

UI: Adding FileDialog UI #832

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

ganimtron-10
Copy link
Contributor

@ganimtron-10 ganimtron-10 commented Aug 4, 2023

FileDialog is an important UI element needed for a variety of use case. Continuing #294 to add the required changes and updates.

TODO:

  • Resize for FileMenu2D. -> in a new PR
  • Resize for FileDialog2D -> in a new PR
  • Fix ListBoxItem issue when adding a new element to listbox.
  • Fix overflowing issue
  • ZeroDivisionError issue

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #832 (b95871b) into master (b29a518) will increase coverage by 0.07%.
Report is 7 commits behind head on master.
The diff coverage is 89.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
+ Coverage   84.49%   84.57%   +0.07%     
==========================================
  Files          44       44              
  Lines       10454    10572     +118     
  Branches     1411     1419       +8     
==========================================
+ Hits         8833     8941     +108     
- Misses       1252     1257       +5     
- Partials      369      374       +5     
Files Changed Coverage Δ
fury/ui/elements.py 90.28% <89.60%> (+0.08%) ⬆️

@skoudoro
Copy link
Contributor

What is the update concerning this PR @ganimtron-10?

@ganimtron-10 ganimtron-10 marked this pull request as ready for review August 16, 2023 14:59
@ganimtron-10
Copy link
Contributor Author

I have fixed the issues which were crucial and needed in this PR.

@skoudoro
Copy link
Contributor

@tvcastillod and @JoaoDell, Can you look at this PR ? Thank you

Copy link
Contributor

@tvcastillod tvcastillod left a comment

Choose a reason for hiding this comment

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

Hi @ganimtron-10, overall it looks like this new FileDialog component is working fine so far.

I just want to understand better which issues you addressed and how did you fix them, for example, for the ZeroDivisionError I saw you fixed it by adding a condition over denom variable, In which cases did this error occur before? or with the Overflow issue, what does it refer to? is it about the file names display?

image

Also, the example looks good, it's simple and easy to understand.

fury/ui/tests/test_elements.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JoaoDell JoaoDell left a comment

Choose a reason for hiding this comment

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

Hey @ganimtron-10, I have just finished reviewing your PR. All the tests passed and the example shows well the feature you worked on. About the scripts, everything seems fine and well organized. Overall looks fine and ready to merge, so good job! I guess I just wanted to better understand the same details @tvcastillod pointed out in her review, as I checked the current folder's path indeed expands out of the textbox. Is it described somewhere in your blogposts?

@ganimtron-10
Copy link
Contributor Author

Hey @JoaoDell @tvcastillod ,
Heres the blog link to get more understanding about what I did in this PR https://blogs.python-gsoc.org/en/ganimtron_10s-blog-copy-2/week-12-filedialog-quest-begins/

@JoaoDell
Copy link
Contributor

Hey @JoaoDell @tvcastillod , Heres the blog link to get more understanding about what I did in this PR https://blogs.python-gsoc.org/en/ganimtron_10s-blog-copy-2/week-12-filedialog-quest-begins/

Just checked your blogpost and I have no more questions. Also I noticed you have fixed the directory textblock clipping, so everything seems in place now, good job

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.

5 participants