#152: Add packing slot/date dividing line

NorwoodAndBrixtonFoodbank / nbf-website

https://main.d3dkw68f839t0q.amplifyapp.com/

5 stars 2 forks source link

#152: Add packing slot/date dividing line #349

Closed KylePUG closed 1 month ago

KylePUG commented 1 month ago

What's changed

Added dividing line between distinct packing slots/dates when sorting by packing slots/dates

Screenshots / Videos

Before	After
	Sorting by Packing Date
	Sorting by Packing Slot

Checklist

[x] The ticket is up-to-date - Please document any deviations from the original approach if there is any.
[x] I have documented the testing steps for QA
[x] I have self-reviewed this PR
[x] Make sure you've verified it works via npm run dev
[x] Make sure you've verified it works via npm run build and npm run start
[x] Make sure you've fixed all linting problems with npm run lint_fix
[x] Make sure you've tested via npm run test:e2e

If you have made any changes to the database... (no changes

AmeliaIsCoding commented 1 month ago

The screenshots look really good, I'm really impressed you managed to do this! however, I think there's been a slight misinterpretation of the ticket. Since sorting by the date means sorting by date, then packing slot, then other fields, I think when we sort by either date or packing slot, in both cases, we should have a dividing line between each packing slot, and between dates. (maybe the line between dates could be thicker or a darker colour than the line between packing slots).

However, your interpretation might be right instead? @rickaw what do you think Dan meant?

KylePUG commented 1 month ago

Made the changes above! Ready to be reviewed again :)

AmeliaIsCoding commented 1 month ago

Made the changes above! Ready to be reviewed again :)

Also btw it's possible to rerequest review using the button on the PR at the top right hand side! :)

KylePUG commented 1 month ago

Did you see the comment I made before my first review, about Dan's intentions with the packing slot/date dividing lines?

Yes. I was waiting for Ricky's response before, but I can just get on this - should not take very long

KylePUG commented 1 month ago

This is how it looks now. I have also changed the getRowBoundaries algorithm as the previous one doesn't work for this additional case :)

KylePUG commented 1 month ago

Solved the desync bug! Ready for review :)