Skip to content

Show archive meetups in reverse chronological order #52

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aolives
Copy link

@aolives aolives commented Nov 14, 2024

Seeing the latest meetup at the top rather than the earliest meetup in the year seemed like it might be a nice change.

Show Archive in reverse chronological order
@aolives aolives marked this pull request as draft November 14, 2024 17:00
@iansltx
Copy link
Contributor

iansltx commented Nov 14, 2024

@aolives Can you rerun the build with this change?

@aolives
Copy link
Author

aolives commented Nov 14, 2024

@iansltx Does that look better? I'm still new to all this.

@aolives aolives marked this pull request as ready for review November 14, 2024 19:14
@iansltx
Copy link
Contributor

iansltx commented Nov 14, 2024

Looks closer at least. I'll take a better look at this over the weekend.

@@ -61,7 +61,7 @@ protected function generateArchivePage(int $year, array $meetups): void
}

$meetups = array_map(fn(MeetupEntry $meetupEntry) => $meetupEntry->instance, $meetups);
rsort($meetups);
usort($meetups, fn($a, $b) => $b->getDateTime() <=> $a->getDateTime());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a sort method on the (doubly-linked list) collection class:

public function sort(): void

That method is already called on the instance that is injected into this class on line 18. So if the collection is already sorted by the time it arrives at this line I'm not sure why rsort doesn't work. A cursory glance has left me more confused than when I started.

Once we get to the bottom of that it'd be nice to have a test around this so we can't break it in the next refactor

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There $meetups is just an array of MeetupEntry rather than the collection.
I think rsort doesn't know we want MeetupEntries sorted by date.

Also, for what it's worth, if I implement the sort on the collection it messes up the order of the RSS feed too which I really don't want to mess with.

@aolives
Copy link
Author

aolives commented Feb 12, 2025

No pressure, but I didn't know if there was anything I was supposed to do with this to move it forward.
Thanks all!

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.

3 participants