Skip to content

Fix/list all events in previous events view #302

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

Merged
merged 1 commit into from
Feb 23, 2025

Conversation

TorgrimRL
Copy link
Contributor

💎 Changes

Pagination for Past Events: Added dynamic pagination with ellipsis, layout adjustments, and CSS tweaks for better alignment.

Screenshot from 2024-10-29 14-40-45

@TorgrimRL TorgrimRL linked an issue Oct 29, 2024 that may be closed by this pull request
Copy link
Collaborator

@otytlandsvik otytlandsvik left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀 I left some comments about a few details you could fix before merging. Also remember not to merge the frontend before the api.

@@ -46,6 +48,11 @@ const EventOverview: React.FC = () => {
const [pastEvents, setPastEvents] = useState<Event[] | undefined>();
const [pastErrorMsg, setPastErrorMsg] = useState<string>('');
const { addToast } = useToast();
const [skip, setSkip] = useState<number>(0);
const [limit] = useState<number>(10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this value is set to 10 and is never changed, it's better to set it to a constant value rather than a useState.

src/api/event.ts Outdated
@@ -55,8 +55,10 @@ export const deleteEvent = (eid: string) => Delete<{}>('event/' + eid);
export const getUpcomingEvents = (): Promise<Event[]> =>
get<Event[]>('event/upcoming');

export const getPastEvents = (): Promise<Event[]> =>
get<Event[]>('event/past-events');
export const getPastEvents = (skip: number, limit: number): Promise<Event[]> =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's generally fine to send limit as a dynamic value here to the endpoint. However, because you're only using a constant value of 10 at the moment, could it be better/easier to simply set it to 10 statically in the endpoint? Both solutions are okay in this case

@@ -132,6 +196,57 @@ const EventOverview: React.FC = () => {
/>
</LoadingWrapper>
)}
<div
className="paginationControls"
style={{ marginBottom: '20px' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, you should choose between providing styles in the stylesheet file eventOverview.scss or using style inline like here. Don't use both className and style on the same element, in general.

i <= Math.min(totalPages - 1, currentPage + 1);
i++
) {
pages.push(i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion, this function would be more readable if the items pushed to the pages list was always of the same data type. So instead of pages.push(i) do pages.push(${i}). (this is nitpicking though)

- Implemented dynamic pagination for 'Tidligere' (past events) with ellipsis for long page lists to enhance user navigation.
- Adjusted component structure to ensure pagination controls render directly under the events list.
- Added conditional rendering and CSS tweaks to handle cases with numerous pages.
- Reduced event overview width for better alignment and display consistency across pages.
- Included error handling and pagination controls styling adjustments for improved UI feedback on navigation buttons.
@otytlandsvik otytlandsvik force-pushed the fix/list_all_events_in_previous_events_view branch from 839fafb to 96e81bf Compare February 23, 2025 14:15
@otytlandsvik otytlandsvik merged commit 00cdfe9 into master Feb 23, 2025
1 check passed
@otytlandsvik otytlandsvik deleted the fix/list_all_events_in_previous_events_view branch February 23, 2025 14:16
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.

List all events in previous events view
2 participants