Revamp Council Page and its components by notkaramel · Pull Request #101 · mcgill-ecsess/ECSESS

notkaramel · 2026-02-03T16:30:44Z

Resolves Revamp to Council Card popup #84

vercel · 2026-02-03T16:30:49Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
ecsess	Ready	Preview, Comment	Feb 10, 2026 5:27am

Copilot

Pull request overview

This pull request revamps the Council page and its associated components to improve the visual design, accessibility, and user experience. The changes resolve issue #84 regarding the Council Card popup.

Changes:

Redesigned council page layout with improved visual hierarchy, section headers, and a responsive grid system
Enhanced modal dialog for viewing council member profiles with better accessibility features
Added new utility function for generating initials from names with fallback support for missing images
Updated CouncilCard and CouncilCardPopUp components with improved styling, image error handling, and responsive design

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
src/routes/council/+page.svelte	Complete page redesign with new layout, modal implementation, keyboard navigation, and improved semantic structure
src/routes/council/+page.server.ts	Updated image resolution from 300px to 360px and added TypeScript type annotation
src/lib/format.ts	New utility function for extracting initials from names with null/undefined safety
src/components/layout/Section.svelte	Added contentStart prop for flexible vertical alignment of content
src/components/council/CouncilCardPopUp.svelte	Redesigned popup with better accessibility, image fallback, and responsive layout
src/components/council/CouncilCard.svelte	Complete redesign with featured/tag support, image fallback, and improved card styling
src/components/council/CouncilAvatar.svelte	Updated styling for consistency with new design system

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/routes/council/+page.svelte

Copilot · 2026-02-03T16:37:35Z

src/routes/council/+page.svelte

+			);
+			return [...list].sort((a, b) => {
+				const aYear = years.findIndex((year) => a.position.includes(year));
+				const bYear = years.findIndex((year) => b.position.includes(year));


The sorting logic for U-reps uses findIndex which returns -1 when a year is not found in the position string. If a representative's position doesn't include any of the expected year markers (U4, U3, U2, U1, U0), they will be sorted to the beginning (since -1 < any valid index). Consider explicitly handling cases where no year is found, either by filtering them out, placing them at the end, or documenting this behavior.

Suggested change

const bYear = years.findIndex((year) => b.position.includes(year));

const bYear = years.findIndex((year) => b.position.includes(year));

// Place representatives without a recognized U-year marker after those with one

if (aYear === -1 && bYear === -1) return 0;

if (aYear === -1) return 1;

if (bYear === -1) return -1;

src/routes/council/+page.svelte

Copilot · 2026-02-03T16:37:35Z

src/components/council/CouncilCardPopUp.svelte

+			{#if image && !imageError}
+				<img
+					src={image}
+					alt={name}


The alt text for council member images simply uses the member's name. For better accessibility, consider making the alt text more descriptive by including context, such as "Photo of [name], [position]" to provide more information to screen reader users.

Suggested change

alt={name}

alt={`Photo of ${name}${position ? `, ${position}` : ''}`}

Copilot · 2026-02-03T16:37:36Z

src/routes/council/+page.svelte

+		return () => {
+			document.body.style.overflow = '';
+		};


The cleanup function in the $effect block always resets document.body.style.overflow to an empty string, even when a modal might still be open. If multiple effects or components manage body overflow, this could cause conflicts. The cleanup function will run when the effect re-runs (e.g., when selectedMember changes from one member to another), briefly resetting overflow before the effect body sets it to 'hidden' again. Consider whether this cleanup is necessary given the effect body already handles both cases explicitly.

Suggested change

return () => {

document.body.style.overflow = '';

};

Copilot · 2026-02-03T16:37:37Z

src/routes/council/+page.server.ts

    position,
    positionDescription,
-    "image": image.asset->url+"?h=300&fm=webp",
+    "image": image.asset->url+"?h=360&fm=webp",


The image height parameter was changed from 300px to 360px. However, the query still specifies only height ('?h=360&fm=webp') without a width parameter. For responsive images, consider whether a width parameter or srcset attributes would be beneficial for optimizing images across different screen sizes, especially since the cards now display at different sizes depending on the 'featured' prop.

Suggested change

"image": image.asset->url+"?h=360&fm=webp",

"image": image.asset->url+"?h=360&w=360&fm=webp",

Copilot · 2026-02-03T16:37:38Z

src/components/council/CouncilCardPopUp.svelte

+	let {
+		name,
+		position,
+		email,
+		positionDescription,
+		yearProgram,
+		image,
+		onClose,
+		id = 'popup-title'
+	} = $props();


The props destructuring lacks TypeScript type annotations, which reduces type safety. Consider adding explicit types, especially since the 'onClose' callback prop is expected but not typed. The type should be something like onClose: () => void to ensure proper contract enforcement.

Copilot · 2026-02-03T16:37:38Z

src/routes/council/+page.svelte

+			role="dialog"
+			aria-modal="true"
+			aria-labelledby="popup-title"
+			onclick={(e) => e.target === e.currentTarget && closeModal()}


The click-outside-to-close functionality uses e.target === e.currentTarget to detect backdrop clicks. While this works, be aware that if the popup content has any margins or if users click between elements in certain edge cases, it might not behave as expected. Consider adding event.stopPropagation() to the inner content div's click handler to ensure clicks inside the modal don't bubble up and trigger closure.

Copilot · 2026-02-03T16:37:38Z

src/components/council/CouncilCard.svelte

+	class="group bg-ecsess-800 ring-ecsess-500/50 ring-offset-ecsess-900 hover:ring-ecsess-300 hover:shadow-ecsess-400/50 focus-within:ring-ecsess-300 focus-within:ring-offset-ecsess-900 grid h-full min-w-0 grid-rows-[auto_1fr] overflow-hidden rounded-xl shadow-md ring-2 ring-offset-2 transition-all duration-500 ease-out focus-within:ring-2 focus-within:ring-offset-2 hover:shadow-xl {featured
+		? 'w-full max-w-56 sm:max-w-64'
+		: 'w-full max-w-56 sm:max-w-64'}"


The conditional expression has identical branches for both featured true and false cases. Both branches return 'w-full max-w-56 sm:max-w-64'. This makes the conditional expression redundant. Either remove the conditional and use a single value, or implement different styling for the featured variant if that was the intention.

Suggested change

class="group bg-ecsess-800 ring-ecsess-500/50 ring-offset-ecsess-900 hover:ring-ecsess-300 hover:shadow-ecsess-400/50 focus-within:ring-ecsess-300 focus-within:ring-offset-ecsess-900 grid h-full min-w-0 grid-rows-[auto_1fr] overflow-hidden rounded-xl shadow-md ring-2 ring-offset-2 transition-all duration-500 ease-out focus-within:ring-2 focus-within:ring-offset-2 hover:shadow-xl {featured

? 'w-full max-w-56 sm:max-w-64'

: 'w-full max-w-56 sm:max-w-64'}"

class="group bg-ecsess-800 ring-ecsess-500/50 ring-offset-ecsess-900 hover:ring-ecsess-300 hover:shadow-ecsess-400/50 focus-within:ring-ecsess-300 focus-within:ring-offset-ecsess-900 grid h-full min-w-0 grid-rows-[auto_1fr] overflow-hidden rounded-xl shadow-md ring-2 ring-offset-2 transition-all duration-500 ease-out focus-within:ring-2 focus-within:ring-offset-2 hover:shadow-xl w-full max-w-56 sm:max-w-64"

Copilot · 2026-02-03T16:37:38Z

src/routes/council/+page.svelte

+			<div class="mt-8 flex flex-wrap justify-center gap-4">
+				{#each vps as vp}
+					<div
+						class="flex min-w-full justify-center sm:min-w-[calc(50%-0.5rem)] lg:min-w-[calc(33.333%-0.67rem)] xl:min-w-[calc(25%-0.75rem)]"


The responsive min-width classes use complex calc() expressions that may not behave as expected in a flex-wrap context. The values sm:min-w-[calc(50%-0.5rem)], lg:min-w-[calc(33.333%-0.67rem)], and xl:min-w-[calc(25%-0.75rem)] attempt to account for gaps, but in a flex container with gap-4 (1rem), these calculations don't correctly align with the actual gap size. Consider using a simpler approach with flex-basis or adjusting the calculations to match the actual gap value of 1rem.

Suggested change

class="flex min-w-full justify-center sm:min-w-[calc(50%-0.5rem)] lg:min-w-[calc(33.333%-0.67rem)] xl:min-w-[calc(25%-0.75rem)]"

class="flex justify-center basis-full shrink-0 sm:basis-1/2 lg:basis-1/3 xl:basis-1/4"

vercel · 2026-02-09T21:46:37Z

Deployment failed with the following error:

Creating the Deployment Timed Out.

notkaramel added 4 commits February 3, 2026 10:17

Redesign some components of the council page

749c204

Redesign the Council cards and popups

2f843ba

Linting of styling issues

32e740c

Linting of styling issues

269754f

notkaramel requested review from MinhVo2005 and Copilot February 3, 2026 16:30

Copilot started reviewing on behalf of notkaramel February 3, 2026 16:31 View session

vercel bot deployed to Preview February 3, 2026 16:31 View deployment

Copilot AI reviewed Feb 3, 2026

View reviewed changes

Adjustment to layout

ecbe872

vercel bot deployed to Preview February 9, 2026 05:14 View deployment

Minor tweak on council card on big vs small screens

664b13b

vercel bot deployed to Preview February 9, 2026 05:24 View deployment

Minor adjustment to the alt text and layout

071ec62

vercel bot deployed to Preview February 9, 2026 05:28 View deployment

Change bottom text about joining ECSESS

5fdf3e0

vercel bot deployed to Preview February 9, 2026 05:30 View deployment

Remove format.ts and changes the layout to fit on small and big screens

c7c44b5

Finishing updating the new council page

9ca8f49

vercel bot deployed to Preview February 9, 2026 22:37 View deployment

Merge branch 'main' into antoine/update-council-page

10bab92

notkaramel closed this Feb 10, 2026

vercel bot deployed to Preview February 10, 2026 05:27 View deployment

notkaramel deleted the antoine/update-council-page branch February 24, 2026 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp Council Page and its components#101

Revamp Council Page and its components#101
notkaramel wants to merge 11 commits intomainfrom
antoine/update-council-page

notkaramel commented Feb 3, 2026

Uh oh!

vercel bot commented Feb 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

vercel bot commented Feb 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-				const bYear = years.findIndex((year) => b.position.includes(year));
+				const bYear = years.findIndex((year) => b.position.includes(year));
+				// Place representatives without a recognized U-year marker after those with one
+				if (aYear === -1 && bYear === -1) return 0;
+				if (aYear === -1) return 1;
+				if (bYear === -1) return -1;

	alt={name}
	alt={`Photo of ${name}${position ? `, ${position}` : ''}`}

	"image": image.asset->url+"?h=360&fm=webp",
	"image": image.asset->url+"?h=360&w=360&fm=webp",

	class="flex min-w-full justify-center sm:min-w-[calc(50%-0.5rem)] lg:min-w-[calc(33.333%-0.67rem)] xl:min-w-[calc(25%-0.75rem)]"
	class="flex justify-center basis-full shrink-0 sm:basis-1/2 lg:basis-1/3 xl:basis-1/4"

Conversation

notkaramel commented Feb 3, 2026

Uh oh!

vercel bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

vercel bot commented Feb 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel bot commented Feb 3, 2026 •

edited

Loading