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

Quick comment navigation #749

Merged
merged 22 commits into from
Jun 20, 2023
Merged

Conversation

yate
Copy link
Contributor

@yate yate commented Jun 19, 2023

This adds two buttons to the bottom of the Post activity for quick parent comment navigation (#542), as well as a Look and Feel setting to toggle them (enabled by default).

It also allows for parent comment navigation with volume buttons. This setting is disabled by default.

device-2023-06-18-230927.webm

@yate yate marked this pull request as ready for review June 19, 2023 03:37
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Solve conflicts, then I'll check this out.

Random thought, but do you think in addition to volume button, a floating action button would work well too?

cc @twizmwazin

@yate
Copy link
Contributor Author

yate commented Jun 19, 2023

@dessalines Thanks, I think this is ready now. Do you mean a floating action button instead of the two buttons added in the BottomAppBar? It looks like this currently.

image

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Nice job, everything works well.

My gut feeling was initially that maybe the index tracker should exist on CommentNodes rather than Post, but considering it should only ever be seen from the post page, and not on other comment lists, its probably better this way.

},
bottomBar = {
if (showParentCommentNavigationButtons) {
BottomAppBar(
Copy link
Member

Choose a reason for hiding this comment

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

Externalize this somewhere, probably in ui/common/appbars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I move scrollToNextParentComment and scrollToPreviousParentComment to Utils as well?
https://github.com/dessalines/jerboa/pull/749/files/573a154d6cb52b624803dc5530a91501d168576a#diff-9958f04ae114c8444c9e781405889c7fab0b5465ca616011fcfb047a243c356bR579

They will be referenced in both the external BottomAppBar and the volume button key event

Copy link
Member

Choose a reason for hiding this comment

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

Sure, up to you.

Icon(
modifier = Modifier.scale(1.5f),
imageVector = Icons.Filled.KeyboardArrowUp,
contentDescription = "Up",
Copy link
Member

Choose a reason for hiding this comment

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

Use i18n strings, lots of examples in this file.

Icon(
modifier = Modifier.scale(1.5f),
imageVector = Icons.Filled.KeyboardArrowDown,
contentDescription = "Down",
Copy link
Member

Choose a reason for hiding this comment

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

same

@twizmwazin
Copy link
Contributor

Solve conflicts, then I'll check this out.

Random thought, but do you think in addition to volume button, a floating action button would work well too?

cc @twizmwazin

A floating action button would be more material-y, but I don't think that's necessarily what users prefer. This feature is an example of porting over a feature users have grown used to using in other apps, and I think in those cases it's important to replicate the UI that they want above all else. If someone wants it we can always add it in the future as an extra option.

yate added 4 commits June 20, 2023 13:10
- i18n comment arrow descriptions
- move CommentNavigationBottomAppBar to AppBars.kt
# Conflicts:
#	app/src/main/java/com/jerboa/ui/components/common/AppBars.kt
#	app/src/main/java/com/jerboa/ui/components/post/PostActivity.kt
@yate
Copy link
Contributor Author

yate commented Jun 20, 2023

I'm noticing a bug where enabling volume button navigation breaks swiping to go back to the previous page, so I need to look into that

@yate
Copy link
Contributor Author

yate commented Jun 20, 2023

@dessalines Ready for another look, thank you

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@dessalines dessalines merged commit 545c9f5 into LemmyNet:main Jun 20, 2023
@yate yate deleted the quick-comment-navigation branch June 20, 2023 20:47
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