-
-
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: Improved Widget code #14458
Conversation
Reduced changed code lines. Mostly renaming and moving classes. Ready for review |
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, works as expected on my computer. I couldn't manage to find any regression.
A second try might be worth.
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.
I'd rename the sidebar from "SideBar" to "Sidebar" but it's optional 🙂
Currently, Files has both SideBar and Sidebar, and the VS spell checker warn you if you use Sidebar, so I decided to use SideBar to create a sense of unity with TabBar. @yaira2 It's what depends on you. |
"Sidebar" |
TabBar is "Tabbar" or "TabBar"? |
"TabStrip" |
What😂 But whatever it'll be, I'm gonna change all SideBar to be Sidebar. |
Things change 😁 |
Ready for review again |
@yaira2 I can revert the last commit to be fetched to the last approved state. I wanna rename to Sidebar in the forth coming PR to merge first. Plus, It turned out that those classes should be prefixed with Standard*Item such as StandardDriveItem and StandardLibraryItem because those classes used as standard encapsulated classes everywhere in Files.App (Standard* respects @lukeblevins's change) |
@0x5bfa can you revert the remaining changes to the |
I'd be curious to know which change it was mentioned from me. 😬 |
You renamed ListedItem to StandardItemViewModel in #12601. I found it was pretty valid and I should rename DriveItem, WslDistroItem and so on like this. |
Summary
1st stage of Widgets cleanup.
Since SideBar and Widget use diffierent data models, added SideBar and Widget prefixes so others could understand what a data model is for easily.
Card means Button.
Container means Expander.
FileTagsContainer means a tag container.
Widget models
Steps To Validate Changes
PR Checklist
Screenshots
N/A
Footnotes
If the request hasn't been agreed by the team, this work might be rejected. Make sure that you get approval from the team before opening any PR related the request. ↩
If you removed any en-US string resources, make sure that there are no references of those resources. When you add a new en-US string resources, do not make any changes on other languages' resources; Crowdin will do that, instead. ↩