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

feat: Redesign the patches screen #2381

Open
wants to merge 10 commits into
base: compose-dev
Choose a base branch
from

Conversation

X1nto
Copy link

@X1nto X1nto commented Jan 6, 2025

This redesigns the patcher screen to reduce clutter. The search bar is now expandable, the patch count is displayed in a bottom bar and the reset button floats neatly above the save button. I added a clear button to the search bar to clear the queries. The filter chips now have a checkmark when selected and the filters have been reworded.

Before After
image image

New patcher screen in action:

Screen_recording_20250109_003652.webm

@Ushie Ushie added the ReVanced Manager Compose Regarding the Compose rewrite of ReVanced Manager label Jan 6, 2025
@Ushie Ushie requested review from Axelen123, Ushie and oSumAtrIX and removed request for Axelen123 January 6, 2025 23:14
Copy link
Member

@oSumAtrIX oSumAtrIX left a comment

Choose a reason for hiding this comment

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

I'll leave a review purely on what I saw in the PR body and leave the rest to the other reviewers:

  • I think in the patching screen the progress bar is at the top, however in the patch selector screen it is at the bottom, perhaps this is a consistency issue we want to avoid
  • The progress bar in the patch selector screen makes it look like you wanna have all patches. "Reaching 100%". However this probably isn't what we want to imply via the UI

LGTM apart from that

@X1nto
Copy link
Author

X1nto commented Jan 6, 2025

I can remove the progress bar from the bottom and keep the x/y selected, or move that to the top too

@oSumAtrIX
Copy link
Member

Satisfies the issues I mentioned 👍

@X1nto
Copy link
Author

X1nto commented Jan 7, 2025

Updated the screenshot and the video showcase according to requested changes

@MarcaDian
Copy link

Is it a big problem to make the number of selected patches as it is done in flutter in the Save button?
It seems to me that UI will be neater.

Screenshot_2025-01-07-15-16-17-029_app.revanced.manager.flutter.jpg

@X1nto
Copy link
Author

X1nto commented Jan 7, 2025

Not a problem at all, as long as everyone agrees

@oSumAtrIX
Copy link
Member

Works for me and avoids this label at the top aligned to the right 👍 LGTM apart from that based on the visuals.

@sexstarvd

This comment was marked as spam.

Comment on lines 275 to 283
val rotation = remember { Animatable(0f) }
LaunchedEffect(searchExpanded) {
val direction = if (searchExpanded) 360f else 0f

rotation.animateTo(
targetValue = direction,
animationSpec = tween(durationMillis = 400, easing = EaseInOut)
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't animateFloatAsState work for this?

val rotation by animateFloatAsState(
	if (searchExpanded) 360f else 0f,
	remember { tween(durationMillis = 400, easing = EaseInOut) }
)

Copy link
Author

Choose a reason for hiding this comment

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

You're right, this is just a leftover from a different type of animation I was testing with. Why remember the animation spec? First time seeing that, not gonna lie

Copy link
Member

Choose a reason for hiding this comment

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

The source code for the animate*AsState functions use remember or global private vals for the default animation spec. Probably doesn't make a difference though.

@X1nto
Copy link
Author

X1nto commented Jan 7, 2025

How do we handle the FAB patch count if the FAB is not expanded?

@oSumAtrIX
Copy link
Member

I think then the count being hidden is fine? If not, maybe disable shrinking (unless that goes against spec)

@Ushie
Copy link
Member

Ushie commented Jan 7, 2025

Just hide it, as it is in the current version

@sexstarvd

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReVanced Manager Compose Regarding the Compose rewrite of ReVanced Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants