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

Migrated subscriptions list screen to Compose #163

Merged
merged 25 commits into from
Aug 21, 2023
Merged

Migrated subscriptions list screen to Compose #163

merged 25 commits into from
Aug 21, 2023

Conversation

ArnyminerZ
Copy link
Member

Depends on #160

@ArnyminerZ ArnyminerZ self-assigned this Jul 20, 2023
@ArnyminerZ ArnyminerZ linked an issue Jul 20, 2023 that may be closed by this pull request
@github-actions github-actions bot added the dependent Depends on other issues/PRs label Jul 20, 2023
@github-actions github-actions bot removed the dependent Depends on other issues/PRs label Jul 28, 2023
@github-actions
Copy link

This PR/issue depends on: * #160

@ArnyminerZ ArnyminerZ marked this pull request as ready for review July 28, 2023 10:42
@ArnyminerZ ArnyminerZ requested a review from rfc2822 July 28, 2023 10:42
@ArnyminerZ ArnyminerZ marked this pull request as draft July 28, 2023 14:44
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Jul 28, 2023
@ArnyminerZ
Copy link
Member Author

Okay, I think now everything is ready

@ArnyminerZ ArnyminerZ marked this pull request as ready for review July 28, 2023 15:18
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Thanks, I tried to accomodate and have moved things around a bit.

  • Settings moved from ui package to main package to distinguish between logic (Settings) and UI (may call Settings to read/change them)
  • MaterialAlertDialogBuilder saves us from implementing the sync interval dialog ourselves.
  • Add TODOs to replace Snackbar by real warning box; again move logic (like calculations when to ask for a permission) to model

@ArnyminerZ
Copy link
Member Author

Maybe for permissions we can use Accompanist. I've used it, and it works like a charm.

@ArnyminerZ
Copy link
Member Author

I've moved the warnings from snackbar to some little cards:

Screenshot_20230821_093821

The issue now is that the "no subscriptions" text is a bit strange with all of this (see screenshot above).

It's technically aligned below the cards, but when there are no cards, it shows up like this:

Screenshot_20230821_095058

I think we should simply not show this message, and even hide the add FAB when the calendar permission is not granted, since synchronization will never run without this. However, maybe we want to allow the user to be able to add subscriptions before granting permissions, if that has any sense, I don't know. Whatever the case, we still have that issue when showing the notification and battery optimization cards. I've thought about aligning it "as a list item" when there are cards, and center it otherwise. But it's open for new ideas.

What do you think? @rfc2822 @devvv4ever

@rfc2822
Copy link
Member

rfc2822 commented Aug 21, 2023

However, maybe we want to allow the user to be able to add subscriptions before granting permissions, if that has any sense, I don't know.

That was the idea. Let's continue rewriting to Compose and then we can still optimize the experience.

@rfc2822 rfc2822 merged commit df4bba1 into dev Aug 21, 2023
7 checks passed
@rfc2822 rfc2822 deleted the compose_iv branch August 21, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite to Jetpack Compose
2 participants