ion
ion copied to clipboard
feat(dashboard): sort archived announcements by expiry date
Closes #1604
- [x] Sort archived announcements by expiry date
- [x] Add pagination
- [x] Implement pagination
- [x] Style pagination
Current Status
Has support for light mode with grey text color
Note: I will rebase with dev
after the PR is approved.
coverage: 79.695% (-0.02%) from 79.718% when pulling 7e09bc58523a5b3cc48552baf6d484463356ad61 on JasonGrace2282:archive into 3feb200726d520ffdc74a303df828caac72682cf on tjcsl:dev.
Is this ready for review? If not, just ping me when it's ready.
@alanzhu0 I think it is ready. I'm looking forward to the feedback!
@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 ;)
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 :)
Sorry for the late review. Good work so far! A few points to fix:
- When you have events that also show as announcements, the following results:
AttributeError: 'Event' object has no attribute 'expiration_date'
. I recommend just sorting byadded
. - 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.
- 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.
my least favorite part: colors!
Are these good?
Looks good! Can you add some spacing around the number input section?
Added this much space:
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?
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:
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?
The UI looks good!
Any final changes you want to make? @krishnans2006 any other comments?
If you're asking me, I just want to get this PR merged xD
Good work!