-
Notifications
You must be signed in to change notification settings - Fork 68
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
Conversation
There was a problem hiding this 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!
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. |
@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.
@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. |
…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.
…ssigning postLocked value.
…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.
@micahmo Made the lock icon dim. |
Perfect! Did you still want to do #648 first, or would you rather this one goes first? |
@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. |
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. 😆) |
@micahmo Looks like there are no conflicts and my lock changes to FAB work seamlessly with your changes :D |
Sweet!! |
@ajsosa Did you intend to move saved to the beginning of the title? Looks like it's still at the end.
|
@micahmo I've never saved a post so had no idea that even existed. But yeah I can move it. |
The padding on the pin icon is also on the wrong side now. |
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
Checklist
semanticLabel
s where applicable for accessibility?