Closed hjiangsu closed 8 months ago
There's still quite a bit to be done here, but @micahmo, could you possibly do some testing on this when you have the chance? It doesn't have to be now, but I just wanted to give a heads up!
I'm sure theres tons of things I'm still missing here to match parity with the current features so it would be good to have someone else test this out and provide some feedback on what's still missing :D
Things that I know of at this time which are still missing:
I think theres some Android specific features that are not yet re-implemented as well (back button related actions)
My goal is to hopefully have the refactor completed by the end of this week if possible. There's still a lot of cleaning up to do after I re-implement all the missing features
@hjiangsu Sure thing, I can start daily driving this branch. I'm happy to live with the quirks for now, and I'll try to only mention things that you haven't already covered.
Here are my first findings in just a few minutes. Overall looks pretty solid and on the right track. As you said, little things here and there need cleaning up.
https://github.com/thunder-app/thunder/assets/7417301/a1b11daf-5003-410d-93fc-5a583b48727c
https://github.com/thunder-app/thunder/assets/7417301/6745b794-79d0-4ae2-95b6-41a2ef846b4e
That's all for now! I think live testing this will be much more fruitful than a code review at this point, so I'm happy to keep running for a while. Also I know this will probably be a bit painful, but good for the overall architecture in the long run!
Thanks for catching those! I'll update those points to my original comment so that I can keep track of them all
That's all for now! I think live testing this will be much more fruitful than a code review at this point, so I'm happy to keep running for a while. Also I know this will probably be a bit painful, but good for the overall architecture in the long run!
Yeah, I don't want anyone to endure this code review (even myself) π
@micahmo I've fixed a lot of the issues that I previously mentioned and that you mentioned! There's still a couple left to do but feel free to do some more testing on it and let me know if you notice anything else
Sounds good, will give it another run tonight!
Regarding this logic...
Platform.isAndroid ? const Icon(Icons.arrow_back_rounded) : const Icon(Icons.arrow_back_ios_new_rounded)
Do you think you could flip it and only use arrow_back_ios_new_rounded
on iOS? I only say that because I think arrow_back_rounded
is the default (for example, if you don't override the leading icon in the AppBar) on all other platforms (i.e., web, desktop).
EDIT
Just gave this another test spin. Looking amazing!! Definitely addressed most of the things I noticed. In addition to my comment above about the back icon selection, here are some more thoughts. (Apologies for piling these on, I'm just writing down what I see! π)
https://github.com/thunder-app/thunder/assets/7417301/737801dd-0d8a-4c38-ac05-a2102f66c059
I don't see taglines, although you checked that task.
The background color when expanding the FAB is not quite enough. It's still hard to read the options. In #706, I had used theme.colorScheme.background.withOpacity(0.95)
.
The sidebar has some quirks.
It fades out at the top and bottom, which looks a little funny. |
---|
I can't tap outside to close it (only swipe).
Scrolling down within the sidebar refreshes the feed (and fails).
https://github.com/thunder-app/thunder/assets/7417301/f649f21d-1f19-4c8a-abd6-248fe95a86d7
There's an unexpected delay after closing the sidebar before the dimmed overlay is removed (and there's no animation).
https://github.com/thunder-app/thunder/assets/7417301/43650b42-5379-48ef-962f-4ff73a6d7ca7
Also the appearance is now very different from the user page.
The buttons (new post, subscribe) don't seem to work.
Communities do not respect the default sort mode.
Communities don't have subscribe button in app bar.
Ok that's all for now! I know it seems like a lot, but the amount of work you've put into this refactor is amazing!!
Do you think you could flip it and only use
arrow_back_ios_new_rounded
on iOS?
Yeah I can do that!
The pull-down refresh icon is now appearing a bit lower than I think it should (in the middle of the community header)
Dang, okay, I'll have to adjust the values again. The positioning of it is controlled by an integer because the refresh indicator is within a CustomScrollView
so its just a matter of tweaking it so that it aligns properly
On the main feed (non-community view), there seems to be a bunch of wasted space before scrolling. Maybe we don't need the cool sliver transition and can just populate the app bar directly?
Yeah, we can do that (or add some more info) like you mentioned! I was just doing some playing around with slivers while doing the refactor π
I don't see taglines, although you checked that task.
Hmm, I tested that with reddthat.com and it was working there, but I can double check
The background color when expanding the FAB is not quite enough.
I'll increase it a bit more!
The sidebar has some quirks.
Yeah... theres still stuff to be done there that I forgot about. I'll continue to fix up the stuff related to the sidebars. The thing is right now, the user page and the feed page are separate pages (user page still follows the original methods) so a lot of things are broken there
I'll go ahead and update the previous comment to add these points in! Thanks :D
This is with 0.95
opacity - do you think this is too much?
do you think this is too much
No, I think it's perfect! For comparison, here's Google Cal.
Just took this for another quick spin to check your latest commit. Good stuff as always. Getting really nitpicky at this point (which means I think it's almost done π)!
https://github.com/thunder-app/thunder/assets/7417301/e804a27c-0478-4041-8beb-4f5c30928f1e
I just wanted to verify that this change is intentional. When the FAB is open, the main (single-press action) stays visible in the button, rather than changing the button into a close button and putting that action in the extended actions. Is that right? If so, I guess we should make a similar page in the post view soon too. π
but now there's some blank space on the home feed between the top post and the top bar. Maybe it's unavoidable without messing up the position of the pull down.
This might be solvable - it could be because of some padding/margins I added to the taglines
I just wanted to verify that this change is intentional. When the FAB is open, the main (single-press action) stays visible in the button, rather than changing the button into a close button and putting that action in the extended actions.
Oh, oops, this was not really intentional π I haven't used the FABs too much myself, so I didn't remember what the default behaviour for that was. Do you think we should keep this change or revert back to the original method?
Do you think we should keep this change or revert back to the original method?
I don't mind the change! It makes sense for the primary action to stay where it is. I guess the only downside is that there's no longer an explicit close button. But I suppose most people know enough to swipe it down or tap outside. I guess I'll leave it up to you. I can see arguments for either way.
I don't mind the change! It makes sense for the primary action to stay where it is. I guess the only downside is that there's no longer an explicit close button.
We will leave it in then, and just make sure that we can close it when we tap outside or swipe down (I think swiping down doesn't work fully at the moment)
On another note, I'm trying to get the community sidebars to work again, but it's been a bit more difficult. I did make some changes which allows it to work at the moment, but it looks different visually. I've expanded the width of the sidebar to take up the whole screen. Personally, I think I like it but I wanted to get some thoughts on this! If we don't like this change, I can continue to work on getting it to match the previous implementation (although it might require some hacky solution)
The main thing thats causing the issues is that I'm mixing both sliver widgets and renderbox widgets. This makes it harder to make the sidebar into an "overlay" where the posts show up behind it. Instead, only a plain background is shown (you can see it briefly when I close the sidebar)
Since this is a visual change, I'll pull in @CTalvio and @machinaeZER0 for their thoughts if they have any!
Oh hey! I don't mind the full-width info pane (seems like a fine change to me), though if the feed can't be animated off and on the screen beneath it during the transition then I think I would have the community info 'blink' in, rather than slide in? If the feed is going to disappear/reappear all at once, I mean.
Also I kinda liked the main FAB action turning into a close button, but also don't feel too too strongly about the proposed new behavior.
Thanks for working on this refactor, gang! Excited for the other opportunities it will unlock in the future :)
Should tapping the community banner have an inkwell effect?
I think I would have the community info 'blink' in, rather than slide in?
Hmm, do you have an example of what you mean by 'blink'?
Also I kinda liked the main FAB action turning into a close button, but also don't feel too too strongly about the proposed new behavior.
I think this can be debated on too! If most people like the existing implementation, then I can revert it back (this was an unintentional change to begin with, so I don't have too much of a preference myself)
Should tapping the community banner have an inkwell effect?
I think this makes sense! I'll add that in as well
My main concern with the full width sidebar, is how it would work on larger devices. We already have dual column feeds, but a lot of other things really aren't very usable on a large device. Even with dual-columns, stuff like comment threads are still full-width and quite awkward. We should look to applications like gmail which make effective use of multiple panes of application content.
Aside from that, I'm quite attached to the overlay look, that's what "sidebar" had always meant to me over the years. I want to be able to open it for any given community from the future pull-up subscription list I want to make (this functionality would make no sense with the current list in the left drawer), or pull it out on the create post screen to check the rules of the community I'm posting to.
My original design for it was made with the intention of being able to summon it in all kinds of contexts.
I would want to keep the main action becoming a close button for the expanded FAB.
We already have dual column feeds, but a lot of other things really aren't very usable on a large device.
Yeah, I agree that we need to rethink how the overall app feels and looks on larger devices. However, to do this, we need to do some major refactoring (starting out with this feed page) so that it's less dependent on other widgets/logic. Once this is done for the major views/pages (feed, post, comment, etc), then we can start applying these views in a more modular way to adapt to larger devices without completely breaking everything.
Aside from that, I'm quite attached to the overlay look, that's what "sidebar" had always meant to me over the years.
I see what the concern is here! Maybe I'll try out a few more tricks/tweaking around to see if I can make it better without adding too much complexity to the code.
I would want to keep the main action becoming a close button for the expanded FAB.
Seems like the majority here goes towards the close button so I'll make that change!
Hmm, do you have an example of what you mean by 'blink'?
It may have been a bug, but in your video the feed was disappearing and then the community panel was sliding into view, which may have just been a bug? In the current release the sidebar animation pops out over the feed, which continues to be drawn.
It sounds like we're probably moving back to an overlay side panel for this element, so my main call-out is making sure the feed doesn't pop out of existence when the sidebar animation starts!
I don't mind the change! It makes sense for the primary action to stay where it is. I guess the only downside is that there's no longer an explicit close button.
We will leave it in then, and just make sure that we can close it when we tap outside or swipe down (I think swiping down doesn't work fully at the moment)
On another note, I'm trying to get the community sidebars to work again, but it's been a bit more difficult. I did make some changes which allows it to work at the moment, but it looks different visually. I've expanded the width of the sidebar to take up the whole screen. Personally, I think I like it but I wanted to get some thoughts on this! If we don't like this change, I can continue to work on getting it to match the previous implementation (although it might require some hacky solution)
The main thing thats causing the issues is that I'm mixing both sliver widgets and renderbox widgets. This makes it harder to make the sidebar into an "overlay" where the posts show up behind it. Instead, only a plain background is shown (you can see it briefly when I close the sidebar)
Since this is a visual change, I'll pull in @CTalvio and @machinaeZER0 for their thoughts if they have any!
Not having an explicit "close" button could screw up closing with talkback
Seems like the majority here goes towards the close button so I'll make that change!
Edit: nevermind. That is what i get for not reading the whole thread before commenting.
I'm just going to add these so that I have someplace to keep track of all the current issues with the refactor since my last commit π« :
@micahmo, would you be able to do a run through of the app to see if there are any more major issues that should be addressed? I think I got most of them, and any of the remaining ones can be addressed in a future PR. I want to get this out if possible because this is already such a large PR that it's hard for me to keep track of all the changes π
I'm sure theres still a lot of cleaning up to do, and I'll try to get to those on an iterative basis. As this is such a big change, I want there to be enough time in the nightlies and pre-releases so that we can catch as many issues as possible before pushing this out to the general releases. I imagine there won't be a general release until we get 0.19.x compatibility regardless so the sooner the better π
A couple of things to note moving forward:
community_bloc
to community_bloc_old
since there are some places which relies on those functions (namely user_page as it has not been transitioned yet)thunder_page
which acts as a way to trigger events "globally" so to speak. I don't think this is the best pattern and I do have some ideas on how to potentially improve this for future iterations. There's probably a whole other bunch of changes that I forgot to list out, but I think those are the main ones!
@hjiangsu Once again, thanks for all the awesome work on this!
I ran through everything we've discussed in this PR so far, and only two main things stand out as still not working. It's totally up to you whether these should be addressed now or later. I think this is otherwise pretty good to go!
I still don't see taglines. I am using programming.dev for reference.
EDIT: Something funky is going on with taglines. If I log into my programming.dev account, then they work, even if I switch back to anonymous mode. But if I remove my account, switch to anonymous, and restart the app, they're gone again. Note that I can't reproduce this prior to the refactor.
https://github.com/thunder-app/thunder/assets/7417301/1d61f863-8823-43a0-a26e-b03657205641
https://github.com/thunder-app/thunder/assets/7417301/202b34e1-cbef-4542-a52e-ff27787f1e52
I'm planning on driving this branch during the day tomorrow in case anything else pops up, but I'd give it the green light otherwise!
Hey @hjiangsu just checking in again after driving this branch for the day. I noticed a couple more things that aren't working quite right, of different severities. Again, totally up to you whether they are addressed in this PR or elsewhere.
There is a gap between some of the icons in the header. |
---|
Hmm, I think I can leave those for a separate PR and just merge this one in! We'll fix those inconsistencies and issues that you mentioned as this one's gotten way too large and out of hand π
Image and link previews cannot be opened from the feed or post. They work fine from comments.
I wasn't able to reproduce this issue - I can tap on a link or image and it opens up the corresponding preview/browser. Could you provide a video of it? Thanks!
I think I can leave those for a separate PR and just merge this one in!
Sounds good!
I wasn't able to reproduce this issue - I can tap on a link or image and it opens up the corresponding preview/browser.
Sure! It's reproducible for me on my emulator and physical device. Here's a recording after getting the latest develop
.
https://github.com/thunder-app/thunder/assets/7417301/953f9a1d-43f6-4061-83c0-d3ab79a9da8b
Without really digging in, it looks like some bloc is missing in the hierarchy.
Could not find the correct Provider<UserBloc> above this LinkPreviewCard Widget
Let me know if you want a hand looking at this since it's kinda broken at the moment. π
Oh I see - it relates to the thumbnail previews being on the right side π I just tested it with it being on the left (and also marking media as read)
Pull Request Description
This is a complete refactor of the feed page (also known as
community_page
). The purpose of this is to:This is currently WIP and is not fully up to parity with the current features. Below will be a list of the features which are currently known which need to be completed
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
semanticLabel
s where applicable for accessibility?