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

Improved cross-post UI #1332

Merged
merged 10 commits into from
May 2, 2024
Merged

Conversation

CTalvio
Copy link
Collaborator

@CTalvio CTalvio commented Apr 28, 2024

Pull Request Description

Improves cross posting UI

Issue Being Fixed

Cosmetics and metadata visibility. Provides similar and additional functionality to #1322

Screenshots / Recordings

Screenshot_20240428_231932
Screenshot_20240428_231924

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

# Conflicts:
#	lib/community/widgets/post_card_metadata.dart
#	lib/post/widgets/post_view.dart
#	lib/shared/cross_posts.dart
Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

Hey @CTalvio, we've missed your contributions! 😊

I love the idea of this, even more than what I was trying to add in #1322. I think all of this extra metadata is critical to deciding whether you're interested in viewing a cross-post.

Here are some of my observations.

  • All of the inkwells have hard corners (we usually use something like a radius of 5 or 10), although an alternative here would be to make the widget edge-to-edge and then it wouldn't need to be rounded).
  • The CrossPosts widget has an inkwell effect even when there's only one cross-post (i.e., it looks tappable, but tapping does nothing).
  • The inkwell around the community name seems to have some space at the end.

And then I just had a couple thoughts to potentially increase the usability of the feature.

  • What do you think about the default view just being a single statement like "Cross-posted to 1 community"/"Cross-posted to 2 communities" with a down arrow that you have to expand. I think that would alleviate some concerns about the metadata being confused with the main post. You would have to expand to see the metadata, even for a single cross-post.
  • What do you think about having the whole row tappable instead of just the community name, so it's more clear that you're navigating to the post and not the community (I think that would be an improvement over the current UX).

Nice work, I think this looks awesome, and I would definitely like to pursue this path over the one I started. 😊

@CTalvio
Copy link
Collaborator Author

CTalvio commented Apr 29, 2024

I glad you approve!

You bring up several thoughts I've had myself since yesterday.

I'll fiddle with this further 👍

@CTalvio
Copy link
Collaborator Author

CTalvio commented Apr 29, 2024

Kooha-2024-04-29-21-39-15.mp4

Now looks like this.

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

This looks absolutely fantastic to me! Thank you so much!

@CTalvio
Copy link
Collaborator Author

CTalvio commented Apr 29, 2024

Hang on. Gonna check how it works in post creation.

@CTalvio
Copy link
Collaborator Author

CTalvio commented Apr 29, 2024

Done

@micahmo
Copy link
Member

micahmo commented Apr 29, 2024

Just FYI, I briefly get an error when loading the create post screen. Then it works fine once the results have come in.

qemu-system-x86_64_SPW5GJy0pR.mp4

@CTalvio
Copy link
Collaborator Author

CTalvio commented Apr 29, 2024

Must have been happening with old crosspost info too, then, but thats an easy fix

@micahmo
Copy link
Member

micahmo commented Apr 30, 2024

Must have been happening with old crosspost info too, then, but thats an easy fix

Probably so. Thanks a bunch!

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

Thanks again for this @CTalvio. I haven't tested this physically but from the demos, it seems to be good to me!

Just have a few comments with regards to the PR itself, but otherwise, looks good to me!

lib/community/widgets/post_card_metadata.dart Outdated Show resolved Hide resolved
lib/community/widgets/post_card_metadata.dart Outdated Show resolved Hide resolved
lib/community/widgets/post_card_metadata.dart Outdated Show resolved Hide resolved
lib/community/widgets/post_card_metadata.dart Outdated Show resolved Hide resolved
lib/community/widgets/post_card_metadata.dart Outdated Show resolved Hide resolved
lib/shared/cross_posts.dart Outdated Show resolved Hide resolved
lib/shared/cross_posts.dart Outdated Show resolved Hide resolved
@CTalvio CTalvio requested a review from hjiangsu May 1, 2024 22:00
Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the changes.

@hjiangsu hjiangsu merged commit c2a71ea into thunder-app:develop May 2, 2024
1 check passed
@hjiangsu hjiangsu mentioned this pull request May 2, 2024
3 tasks
@CTalvio CTalvio deleted the improve_cross_posts branch May 3, 2024 15:30
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.

None yet

3 participants