ion icon indicating copy to clipboard operation
ion copied to clipboard

feat(dashboard): sort archived announcements by expiry date

Open JasonGrace2282 opened this issue 3 months ago • 5 comments

Closes #1604

  • [x] Sort archived announcements by expiry date
  • [x] Add pagination
    • [x] Implement pagination
    • [x] Style pagination

Current Status

dark mode pagination Has support for light mode with grey text color

Note: I will rebase with dev after the PR is approved.

JasonGrace2282 avatar Mar 21 '24 18:03 JasonGrace2282

Coverage Status

coverage: 79.695% (-0.02%) from 79.718% when pulling 7e09bc58523a5b3cc48552baf6d484463356ad61 on JasonGrace2282:archive into 3feb200726d520ffdc74a303df828caac72682cf on tjcsl:dev.

coveralls avatar Mar 21 '24 18:03 coveralls

Is this ready for review? If not, just ping me when it's ready.

alanzhu0 avatar Apr 02 '24 02:04 alanzhu0

@alanzhu0 I think it is ready. I'm looking forward to the feedback!

JasonGrace2282 avatar Apr 02 '24 12:04 JasonGrace2282

@alanzhu0 @krishnans2006 I believe I fixed most of your initial concerns, would appreciate another look at this PR when you guys find the time.

Sorry for taking so long, was occupied with a PR on another repo ;)

JasonGrace2282 avatar Apr 06 '24 02:04 JasonGrace2282

not sure if we want something implemented for the "show all" pages as well.

I'm not sure what you mean, for me regardless of whether show_all=1 pagination occurs. Is this not the expected/wanted behavior? Actually that made me think, what is the expected sorting behavior when show_all=1? Right now it sorts by pinned and then added date, should it sort by expiration date first instead?

Other than that, the PR should be ready for another round of review :)

JasonGrace2282 avatar Apr 06 '24 16:04 JasonGrace2282

Sorry for the late review. Good work so far! A few points to fix:

  1. When you have events that also show as announcements, the following results: AttributeError: 'Event' object has no attribute 'expiration_date'. I recommend just sorting by added.
  2. Small aesthetic stuff: could you move this onto one line? Also, could you make the selected page have a highlighted background color instead of the blue text color? I think that would look better. image
  3. Lastly, this is applying well to the archive view, but can you also apply this same logic for the admin "Show All" view?

In regards to @krishnans2006 's comment, your sorting behavior is good.

alanzhu0 avatar May 01 '24 19:05 alanzhu0

my least favorite part: colors! dark mode light mode Are these good?

JasonGrace2282 avatar May 01 '24 20:05 JasonGrace2282

Looks good! Can you add some spacing around the number input section?

alanzhu0 avatar May 01 '24 23:05 alanzhu0

Added this much space: Full bottom bar

JasonGrace2282 avatar May 02 '24 01:05 JasonGrace2282

Sorry about being picky about this, one more suggestion. It looks a bit weird floating somewhat in the middle. How about removing the "Older posts" and "Newer posts" buttons and replacing them instead with left and right arrows, before and after the page buttons? So, something like [<] [1] [2] [3] [4] [>]. And then, the number input could be right-aligned to where the "Older posts" button is right now. Finally, since we have a lot of space, could you increase the number of pages shown? Maybe like ten total?

alanzhu0 avatar May 02 '24 13:05 alanzhu0

image Is this what you're looking for?

Regarding the comment about 10 items in pagination, when you reach higher pages it starts to look like this: image I think this takes up enough space. If you want, I could reduce the surround pages (like to ±1) and increase the beginning/end pages?

JasonGrace2282 avatar May 02 '24 15:05 JasonGrace2282

The UI looks good!

alanzhu0 avatar May 02 '24 18:05 alanzhu0

Any final changes you want to make? @krishnans2006 any other comments?

alanzhu0 avatar May 02 '24 18:05 alanzhu0

If you're asking me, I just want to get this PR merged xD

JasonGrace2282 avatar May 04 '24 15:05 JasonGrace2282

Good work!

alanzhu0 avatar May 04 '24 20:05 alanzhu0