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

SMA-275: Redesign the settings screen to get a better alignment with iOS #22

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

qnga
Copy link
Collaborator

@qnga qnga commented Apr 2, 2022

@qnga qnga requested review from robbyice and MalcolmMcFly April 2, 2022 07:02
@MalcolmMcFly
Copy link
Contributor

@qnga can you attach a screenshot of this?

@qnga
Copy link
Collaborator Author

qnga commented Apr 11, 2022

@robbyice
Copy link
Collaborator

robbyice commented Apr 11, 2022

This overall looks good but I have a few notes:

  • We should endeavor to use DialogFragments for dialogs whenever possible. This might be a good opportunity to make that change.
  • The SR2SettingsDialog seems like a good candidate for view and/or data binding
  • If you're so inclined, you can use the coroutines-rx2 library to convert an observable to a flow. This way we can keep slowly moving the project over to fully use coroutines/flows/etc.
  • Adding some tests here could be good as well (also testing will be easier with when using a DialogFragment)

@qnga
Copy link
Collaborator Author

qnga commented Apr 12, 2022

  • I like that the dialog is being dismissed on orientation change. That would not happen with DialogFragment whose I think it's the main feature.
  • There is no significant advantage, so I preferred consistency. I consider this repo to be in maintenance mode.
  • I'm not used to UI tests and not convinced they are a great thing. Mocking-based fragile tests in core make every change time-wasting and very painful, and I believe that UI tests are even more fragile. Besides, I manually tested all features in this dialog and as it's an independent piece of code, it shouldn't be broken accidentally by anything else. Feel free to try to convince me if you like.

@MalcolmMcFly MalcolmMcFly changed the title SMA-215: Redesign the settings screen to get a better alignment with iOS SMA-275: Redesign the settings screen to get a better alignment with iOS Apr 12, 2022
@robbyice
Copy link
Collaborator

I think wanting the dialog to dismiss on orientation change is reasonable. Using DialogFragment would protect it against any configuration change, which includes other situations, but it's probably not necessary if we're not worried about ensuring the dialog stays open.

I'd like to move the codebase towards consistency around new, more modern Android patterns, rather than consistency with things as they are. I don't think it's a requirement for this ticket but it's something to think about.

I'm happy to have a separate conversation about the benefits of testing. UI testing in Android has indeed been historically fragile, but the ecosystem is significantly better nowadays. There are some good examples of unit tests for fragments/viewmodels in the core repo.

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

Successfully merging this pull request may close these issues.

3 participants