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

#665: Indicators for locked posts + reply prevention #676

Merged
merged 9 commits into from
Aug 24, 2023

Conversation

ajsosa
Copy link
Collaborator

@ajsosa ajsosa commented Aug 22, 2023

Pull Request Description

Added a lock icon prefixing post titles for locked posts in both comfortable and compact view. Also replaced reply button in post view with lock as well as the FAB reply button. Commenting operations are blocked and a toast will notifying explaining that the post is locked.

Issue Being Fixed

#665

Screenshots / Recordings

snippet1
snippet2
snippet3
snippet4
snippet5

Checklist

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

@ajsosa
Copy link
Collaborator Author

ajsosa commented Aug 22, 2023

@micahmo It just occurred to me that these changes will probably have merge conflicts with #648. I'm cool with reviewing and merging your stuff first and then fixing my PR.

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.

LGTM, just left a couple minor comments. About the conflicts, yeah you're probably right. Mine's pretty big but you're welcome to review if you want!

lib/community/widgets/post_card_view_comfortable.dart Outdated Show resolved Hide resolved
lib/post/pages/post_page.dart Outdated Show resolved Hide resolved
@CTalvio
Copy link
Collaborator

CTalvio commented Aug 23, 2023

Are we keeping the lock icons at the start of titles, and pin icons at the end? That seems inconsistent. Either change locked to be at the end, or pins and favorites to also be at the beginning.

@ajsosa
Copy link
Collaborator Author

ajsosa commented Aug 23, 2023

@CTalvio Yeah I noticed that yesterday. I think I would prefer both to prefix the title so that their placement is always consistent.

…st page. Also prevents you from attempting to comment on a locked post.
@micahmo
Copy link
Member

micahmo commented Aug 23, 2023

@ajsosa One thing I forgot to mention on the topic of the other indicators. Make sure the lock icon dims when the post is read. See here for example.

https://github.com/thunder-app/thunder/blob/develop/lib/community/widgets/post_card_view_comfortable.dart#L127

…ked posts. Added localization string for reporting that a post is locked and comments are disabled. Added a toast notification when user pressed the lock on the post or the FAB indicating that replies are not allowed on locked posts.
…ck icon for locked posts. Also fixed issue of featured posts not making the title green in comfortable mode and show title first set to false.
@ajsosa
Copy link
Collaborator Author

ajsosa commented Aug 23, 2023

@micahmo Made the lock icon dim.

@micahmo
Copy link
Member

micahmo commented Aug 23, 2023

Perfect! Did you still want to do #648 first, or would you rather this one goes first?

@ajsosa
Copy link
Collaborator Author

ajsosa commented Aug 23, 2023

@micahmo I'll try to get to reviewing your changes later today. But if you would prefer to merge my stuff in first that's fine too. your call.

@micahmo
Copy link
Member

micahmo commented Aug 23, 2023

I don't have a strong preference, so I guess we can just go in the order they were created if that's ok. (And I promise it's not just because it benefits me this time. 😆)

@ajsosa
Copy link
Collaborator Author

ajsosa commented Aug 24, 2023

@micahmo Looks like there are no conflicts and my lock changes to FAB work seamlessly with your changes :D

@micahmo
Copy link
Member

micahmo commented Aug 24, 2023

Sweet!!

@micahmo micahmo merged commit 82c709e into thunder-app:develop Aug 24, 2023
1 check passed
@micahmo
Copy link
Member

micahmo commented Aug 24, 2023

@ajsosa Did you intend to move saved to the beginning of the title? Looks like it's still at the end.

image

@ajsosa
Copy link
Collaborator Author

ajsosa commented Aug 24, 2023

@micahmo I've never saved a post so had no idea that even existed. But yeah I can move it.

@CTalvio
Copy link
Collaborator

CTalvio commented Aug 24, 2023

The padding on the pin icon is also on the wrong side now.

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