-
-
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: Introduce details layout columns generator phase 1 #14127
Code Quality: Introduce details layout columns generator phase 1 #14127
Conversation
Looks good behavior wise. I'd like to wait for an additional review from @gave92 or @hishitetsu before merging. |
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.
Great work! 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.
GetHashCode()
can be improved in the same way as LayoutPreferencesItem
.
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.
Fixed already in the stage 2 PR. Thanks!
Summary
As of now, you have to add literally 250 lines only for one column when you are adding a new one (you can see git column introduction PR). We are introducing columns generator to mitigate that situation.
Task Checklist
Steps To Validate Changes
If nothing happen, you confirmed layout settings for each page are stored correctly in user_settings.db, hide/show operation is working properly, the app is able to fetch settings from the database and use them in an actual layout.
PR Checklist
Screenshots
Related codebase structure:
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. ↩