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

Allow editing posts #853

Closed
wants to merge 11 commits into from
Closed

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Oct 25, 2023

Pull Request Description

This is the beginning of an edit posts feature, and it works pretty well. However, posts edited through the user profile do not update, because they are not using the same FeedBloc that the events go through. Therefore, it may be smart to leave this feature until the user page is refactored to use the same widget/bloc. Leaving this as a draft for now. See next comment.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

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

@micahmo
Copy link
Member Author

micahmo commented Oct 31, 2023

I'm going to mark this ready for review, since I think it works as well as it can with the current architecture. Here is a demo of editing a post from a community feed, and it works fine. It also works fine from the user feed, and you can see the post itself being updated in realtime. The only small thing that doesn't work is that the post doesn't get updated in the user feed listing. I think that would work fine once we migrate the profile view to use the new FeedBloc, but I didn't want to wait for that refactor (which will probably be pretty big) to get this feature in. Hope that's ok!

Review without whitespace!

qemu-system-x86_64_MoUMirUjME.mp4

@micahmo micahmo marked this pull request as ready for review October 31, 2023 19:42
@micahmo micahmo requested a review from hjiangsu October 31, 2023 19:42
@hjiangsu
Copy link
Member

The only small thing that doesn't work is that the post doesn't get updated in the user feed listing. I think that work fine once we migrate the profile view to use the new FeedBloc, but I didn't want to wait for that refactor (which will probably be pretty big) to get this feature in. Hope that's ok!

I'm actually working on the refactor for the user page as we speak (still a WIP so not expected to be done yet)! I'll go ahead and take a look at the PR and provide any feedback if needed. Thanks!

@micahmo
Copy link
Member Author

micahmo commented Oct 31, 2023

I'm actually working on the refactor for the user page as we speak

Oh sweet! Looking forward to that! Then I'll leave it up to you as to which goes first. If you decide to merge this one first, there's just one line with a TODO that can be updated once the refactor is done.

@hjiangsu
Copy link
Member

hjiangsu commented Nov 1, 2023

So I took a quick look at the changes, and I'm wondering if we can perhaps approach this in a different way. Right now, it feels like CreatePostPage is tightly coupled with a lot of other components since it has to add/update the corresponding post on the Feed, Inbox, PostPage, etc.

I'm thinking we can decouple CreatePostPage from all the other components/blocs by doing something like this:

  • CreatePostPage will have it's own bloc that gets initialized whenever we navigate to the CreatePostPage. This bloc will be in charge of handling the internal state of CreatePostPage. This includes things like uploading image, creating a new post, editing an existing post, handling errors, saving and retrieving drafts, etc.
  • Instead of passing blocs around, we have a callback function onPostSuccess(PostView postView) on CreatePostPage which gets called when the post gets created or updated properly. This callback function will pass back the new/updated post which is then handled by the calling function

An example of how this would work:

// From some arbitrary component that we need to navigate to the CreatePostPage (e.g., FeedPage, InboxPage, PostPage)

void _updatePostView(PostView postView) {
  // If this is the FeedPage
  // Convert postView to PostViewMedia
  context.read<FeedBloc>().add(FeedItemUpdatedEvent(postViewMedia)
}

navigateToCreatePostPage(
  // Pass in any required values
  onPostSuccess: (PostView postView) => _updatePostView(postView);
)

Note that this is using PostView, but we can use PostViewMedia depending on what works best!

This same concept could then be applied to CreateCommentPage as well. This way, most of the logic is contained within CreatePostPage/CreateCommentPage, and it would be less dependent on other blocs. This is just an idea, so let me know your general thoughts on this!

@micahmo
Copy link
Member Author

micahmo commented Nov 1, 2023

That sounds like a very reasonable approach! Is that something you'd be willing to take on after the current user/feed refactor? It sounds like you have a pretty good plan for what should go where so I'd hate to deviate from that. 😊

@hjiangsu
Copy link
Member

hjiangsu commented Nov 2, 2023

I don't mind either way. If you'd like to work on it, by all means go ahead! The main point from my previous comment was to reduce dependencies on other blocs whenever possible 😄

@hjiangsu
Copy link
Member

I'll close this in favour of #893!

@hjiangsu hjiangsu closed this Nov 20, 2023
@micahmo micahmo deleted the feature/edit-post branch November 22, 2023 15:44
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.

2 participants