-
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
User feed page refactor #1144
User feed page refactor #1144
Conversation
Good luck! 😊😊 |
…oc to match feed_bloc and community_bloc
@micahmo I think this is functional enough for you to try it out! I sure I missed something during my implementation so let me know if you notice any weird behaviours. I mentioned some caveats I found while testing this myself (will most likely attempt to fix the first and second bullet points in separate PRs later) Also, no need to do any code reviews or anything since there's been a lot of changes 😅 |
Just because of time I wish I had but don't, I would highly recommend merging the read-on-scroll branch before this one, it looks like there will be conflicts here, and you'll be much more suited to understanding them than me. |
I agree! I'll be merging in all other PRs before I merge this one in to minimize as many conflicts as possible from your end 😄 This is still in draft too since I feel like there's more testing to be done |
Interesting! I copied the behaviour of the community header, so that might have something to do with it. Maybe I can change it here so that it scales the text down based on the length of the name.
Hmm, I'll take a look at that!
Yeah, I wasn't sure of a good way to do it without overcomplicating the logic. This is because the tabs are not technically a part of the header, so when the sidebar shows up, it shows up under the tab bar (which I think looks more awkward to say the least) If you have any suggestions, let me know!
The endpoint that we use to retrieve the user posts/comments are the same, and it seems like it takes in the sorting of the post 😅 I think the Lemmy backend handles this some way but I'm not too sure myself |
Alright, I've updated this a bit to fix some of the issues:
This might be due to the SizedBox that is at the end of the sidebar to allow it to scroll up more. I don't think this affects functionality per-say, so I left it as is. |
Oh yeah, that looks awesome! Otherwise I think this is good to go! |
I was a bit torn on this. I tried to make the full name fit in one line, but that made it difficult to read for some long usernames. Using ellipses makes it so that we can't fully read the name which is also something I'd like to avoid. I think we can leave it as is for now and see if this is an issue?
Nice! I'll wait until I can merge in some other PRs first (e.g., mark as read on scroll) and then merge this in! |
Sounds good! By the way, I compared against the current behavior. It looks like the short username is truncated with a fade out and the full name is truncated with ellipses. Wrapping is fine for now; maybe we can think of something more elegant in the future. 😊 |
I'll go ahead and merge this in! Let me know if you find any additional issues while running with this change 😄 |
Woohoo! |
Pull Request Description
This is the second attempt at refactoring the user feed page. This time, I've decided to limit down the scope of implementation to be concentrated on the feed (rather than both the feed and account pages). Because of this, there are some files which I've renamed (as they'll be deprecated in the future) to allow the account page to use the previous logic.
Caveats:
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
semanticLabel
s where applicable for accessibility?