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

refactor(mobile): app settings #7749

Merged
merged 5 commits into from
Mar 12, 2024
Merged

refactor(mobile): app settings #7749

merged 5 commits into from
Mar 12, 2024

Conversation

shenlong-tanwen
Copy link
Member

Changes made:

  • Refactored the mobile app settings into sections
Settings Timeline settings
settings timeline_settings
Landscape Settings Landscape Light theme
horizontal_advanced horizontal_light

Copy link

cloudflare-pages bot commented Mar 8, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 59037fc
Status:⚡️  Build in progress...

View logs

@alextran1502
Copy link
Contributor

Hi @shenlong-tanwen, thank you for the PR. The separation is nice! I changed the styling of text a little bigger to make things easier to read.

One bug I found is that changing the value inside Timeline settings doesn't change the grid behavior. For example, change the group by effect or Number of assets per row doesn't actually change the behavior of the timeline. Probably the changes don't get propagated to the AssetGrid level

shenlong-tanwen and others added 2 commits March 11, 2024 22:08
* refactor: SettingsButtonListTile

* refactor: Backup settings to App settings

---------

Co-authored-by: shenlong-tanwen <139912620+shalong-tanwen@users.noreply.github.com>
@shenlong-tanwen
Copy link
Member Author

One bug I found is that changing the value inside Timeline settings doesn't change the grid behavior. For example, change the group by effect or Number of assets per row doesn't actually change the behavior of the timeline. Probably the changes don't get propagated to the AssetGrid level

Great catch. Fixed now :) The issue was that we used to invalidate the appSettingsServiceProvider on timeline settings change which resulted in the main timeline getting rebuild. This was missed in the refactor so the change was pushed up until the next rebuild. Fixed now by invalidating the provider

@alextran1502 alextran1502 enabled auto-merge (squash) March 12, 2024 14:50
@alextran1502 alextran1502 merged commit 7489db9 into main Mar 12, 2024
23 of 24 checks passed
@alextran1502 alextran1502 deleted the refactor/mobile-settings branch March 12, 2024 14:56
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.

None yet

2 participants