-
Notifications
You must be signed in to change notification settings - Fork 32
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
Issue 496/add recent activity page #537
Issue 496/add recent activity page #537
Conversation
…ge' into issue-496/add-recent-activity-page
…e-496/add-recent-activity-page
Okay, this is still a draft PR because I can see comments in the Cyrillic alphabet ;) but I had a rough read through! I like that all the main pieces of the solution seem to be there, and that you were able to use the API to get the required data. What was the reason to fetch locations by ID? We should not need to do two API calls for something that can be a database join and, if it's just one or two fields that are missing it's much better if we add it to the API payload - you can open a ticket in https://github.com/falling-fruit/falling-fruit-api and stub out the data for now. Either Ethan will add it, or I'll be able to have a go myself. Instead of changing state.list and state.location (which hold data needed the list page and location page respectively), it's going to be much better for maintainability if you define a new slice, state.activity, and keep the state there: it will look like
in the component. The async / promise chaining code should also go there, and ideally your component will mostly be rendering and doing UI code. We would also benefit if the organization of the changes follows a few other conventions in the code base:
Can you have a look at existing usages of |
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.
Marking the PR as not ready yet - see comments on the issue and in the PR
Also, this encouraged me to go with an infinite list for #525 ! I had a thought @paskal98 , you could have a look at the code organization for the list module, I think with what the students originally did + how I changed it, it makes a lot of sense. The redux module offers "start/extend" reducers to show the start and end, there's a ListPage file which handles loading / unavailable scenarios, and Locations which is responsible for the display. There actually isn't a 'src/components/connect' module for the list because there's an obvious place to put the effect in - the list page - whereas for e.g. the localized types there isn't, so my ConnectTypes example wasn't that good. Also, if you have any suggestions for me on the list changes, I'll appreciate them - feel free to add yourself as a reviewer on my PR. |
Nice! I checked out the newest branch you pushed -d619c41 which still has "activity" in its own section on mobile so you have probably have something newer locally. First comment, something strange happens to location links:
The intention to group by type and user is great, but the links need to be correct. Maybe in the case of multiple locations the line should say "{number of locations} locations" and end with a chevron right, that on click unfolds to e.g.
I also spotted this one, it is not correct: because I've edited one location twice. The scrollbar jumps when the list gets extended, and there's no visual cue that more element will happen. Try add a skeleton entry - header and a few lines of grey bars, see Skeleton in the code and its uses - and extend the list day by day, even if it is fetched in larger chunks? |
Also, a code suggestion, if you're making transformations on the API payload and they are getting quite complex, you can dedicate a module in src/utils to just that, and use Typescript. Define a data structure that is easy to display, and have the type checker help you with functions that convert the API data structure into one you need. I did something like that in |
@paskal98 I finally had a chance to take a look at the exciting developments in this thread! A few thoughts:
Otherwise, I think it looks great and I'm looking forward to merging! |
…ry commas/dashes, improved object grouping, added loading indicators, updated title/URL, optimized list expansion with API debounce and DOM limit
Hi, i added new updates
|
Awesome, this looks really good! Thanks again for working on this. Being able to scroll down and see what people added is really fun. 🐛 After scrolling down and then up, I end up at this view: 🐛 Comparing the list to https://fallingfruit.org/changes reveals some missing content - looks like you're accidentally skipping locations that don't have a user or an address. 🦋 I like separating different locations with a black comma. If a user adds or edits a location with multiple types, for example this one I changed a week ago:
could we show it as one link - with the comma also blue, and underlining together on hover? 🦋 I like the typography and the fact that the site reads well on both desktop and mobile, in a variety of screen sizes 🦋 I like the skeleton that extends the list and communicates that something is loading! The initial skeleton could be bigger to cover most of the screen - say three sections with five to seven days each - while the little skeleton at the end is great. That's a really minor change so only do it if it sounds inspiring |
Thanks for explaining, I've made a ticket in the API repo. I think we can merge without that! Would you like to address the other bugs / suggestions before we merge? |
So far, this is the only bug I have found 🐛 As for suggestions, I would modify the API for /locations/changes to include the ability to search by date parameters “start” and “end” This would be a good optimization for both the list and future functionality. |
Updated the API ticket, thanks! What would you like me to do with this PR for now? |
I'm currently adding scroll-up functionality with skeleton loading for optimization and making minor updates. I plan to complete the work by the end of the day and would appreciate it if you could merge it afterward 😊. If any new bugs occur, I hope my implementation is stable, but if anything does come up, I will be happy to fix them as a new ticket |
I've added minor updates, but it seems that optimizing the infinite scroll will need more time to explore and discuss 🥲. It would be better if we could create a new ticket for that and close this one, if possible 🙂 |
You could leave it for a while if you're fed up with it, or I can have a go at finishing the PR for you? It was a big piece of work, and you've done it the hard way (also reconsidering I've looked at the code and there are |
Sorry, my baby sent a comment and closed the PR! I was gonna say, I can finish it for you if you like. Apart from the bug / missing feature with the list not coming back up, I also see some changes to the code that will be good for long-term maintenance of this feature:
|
I will continue 😁 It's interesting for me. I just need a time to research how provide optimisation for infinity scrolling. It seems no good solution using react-windows, because of items of list are always have different size, and using react window virtualisation may cause some new bugs. Other way using chunks, today I try added this one, but I struggle with not proper way of chunking data and other bug appears with not smooth scrolling. Additionally i added reducers for maintaining rerendering, it's seems to be good solution but with chunking it was laggy sometimes. So that is way I thinking about closing this issue , and opening others one. Just for delegation work. Provide good optimisation seems can take more time that we can expected Additionally I wanna thank you for supporting and feedbacks! |
I agree with you that since react-window asks for fixed (or at least known) element sizes it is not the right tool here. I think how you're solving the virtualization is fine - with an IntersectionObserver and an upper limit of displayed entries so the page doesn't get infinitely long - it just needs another piece so the user sees something valid after scrolling back up. Or take a step back and just let the page get longer and longer. When I'm not sure how to proceed with something, I find it often helps me to revisit the existing code and try to restructure it or simplify it - "make the change easy, then make the easy change" - and it's especially true since the availability of AI tools like Aider. So if I was to continue this work, I would start from that - for example, making a separate redux slice for this functionality will definitely be good. |
@paskal98 let's finish this PR together :)! If you're still interested in it, you could bring the branch up with the work we did since:
I can take it on once I'm done with #383, and can then take it over from you up to the finish line. We'll be sending out a newsletter soon and tell users we're developing the beta site, I think this is a really cool feature and a good showcase for what can be achieved with React vs. server side rendering on the live site, so it'd be neat if we had it working. |
Okay😊 |
All right, I'm taking over! I'll open another PR when I'm done and add you as a reviewer. |
Added activity page with infinity scroll list
Closes #496