-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Code Quality: Refactored tab control #13197
Code Quality: Refactored tab control #13197
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the code looks good, except in a few cases.
You might consider removing using Files.App.UserControls.TabView
from most of the files.
I'm not a big fan of the new class names, they are in conflict with existing ones. They might confuse other contributors
src/Files.App/Data/Contexts/Multitasking/MultitaskingContext.cs
Outdated
Show resolved
Hide resolved
Does anyone have an idea? |
CustomTabView will do. |
This one sounds good |
@yaira2 do you have plans to create custom template tabview? it is supposed to be easier than sidebar tbh. |
@0x5bfa you mean a custom control like we did with the sidebar? It's something I'd like to do at some point but there is a lot of logic (particularly around resizing the tabs) so it's quite a large task. |
Ready for review. @yaira2 |
@yaira2 I can rename CustomTabView with something else if you dont like it such as MultitaskingTabView. |
@0x5bfa what's in scope for this PR? |
|
Unwrapped the content frame from usercontrol, so slight performance improvement will be expected. |
How about HorizontalTabView? |
If we are certainly going to implement vertical one. But if not, I don't like it. |
Vertical tabs aren't in the plans |
Then adding Horizontal is not appropriate I believe. |
What about TabBar? |
Sounds great. Would you like me to change to be it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@0x5bfa can you look into the build error? |
@0x5bfa can you also share some steps to test these changes? Ideally this would include all the different functionality around the tab bar. |
No changes were made of functionalities. But cleaned up codebase to make them have clear purpose of existence. Just opening the app would check if this changes were appropriately made. |
…/0x5bfa/Files into 5bfa/Cleanup-MultitaskingControl
Please take another look. |
Done |
@0x5bfa looks good, thank you for the PR. |
Code Quality: Clean up multitasking control
I'm actually surprised how the code was complicated. This would be a milestone for implementation of the templated custom tabview for the future.
Motivation & Context
PR Checklist
Related Feature: Improving Performance, optimizations, reliability and codebase #4180
Related Code Quality: Continued work to clean up entire project #12340
Screenshots