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: split settings into smaller components #1320

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented Jun 28, 2024

Split our growing Settings component into smaller, more focused components.

  • Appearance Settings
  • Notification Settings
  • System Settings
  • Settings Footer

@setchy setchy added the refactor Refactoring of existing feature label Jun 28, 2024
@setchy setchy added this to the Release 5.10.0 milestone Jun 28, 2024
Copy link
Collaborator

@bmulholland bmulholland left a comment

Choose a reason for hiding this comment

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

It's a nice cleanup, thank you!

I do wonder if these per-component test files are more effort than they're worth. Integration-level tests could cover all of these, pretty much, without needing to have a test-per. As it is, I feel that the tests make the system harder to change without contributing much resiliency to errors.

@setchy
Copy link
Member Author

setchy commented Jul 1, 2024

I do wonder if these per-component test files are more effort than they're worth. Integration-level tests could cover all of these, pretty much, without needing to have a test-per. As it is, I feel that the tests make the system harder to change without contributing much resiliency to errors.

I would argue the opposite. They're easy to write and easy to make changes. I haven't had any concerns

@setchy setchy merged commit 1d10aef into main Jul 1, 2024
8 checks passed
@setchy setchy deleted the refactor/settings-components branch July 1, 2024 12:18
@bmulholland
Copy link
Collaborator

Oh cool, good news!

@afonsojramos
Copy link
Member

It would have been way easier to merge this after #1318 :(

@afonsojramos
Copy link
Member

Nevermind, it was rather easy afterall 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants