Skip to content
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

feat: ✨ create group page #30

Closed
wants to merge 8 commits into from
Closed

feat: ✨ create group page #30

wants to merge 8 commits into from

Conversation

adi-lux
Copy link
Contributor

@adi-lux adi-lux commented Feb 1, 2024

Creating groups page with 1. users in group and 2. meetings involving the group

@adi-lux adi-lux linked an issue Feb 1, 2024 that may be closed by this pull request
@MinhxNguyen7
Copy link
Member

How's progress on this? Is it ready for review?

@adi-lux
Copy link
Contributor Author

adi-lux commented Feb 8, 2024

How's progress on this? Is it ready for review?

I still want to make some more changes, but I'll let you know by meeting

@MinhxNguyen7
Copy link
Member

Clicking on the meeting should always take you to the meeting setup page.

@MinhxNguyen7 MinhxNguyen7 changed the title feat: ✨ created member types for groups page feat: ✨ create group page Feb 12, 2024
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

Code mostly looks fine with some suggestions.

Also, please write the PR description such that I know how to test the feature and how it's supposed to be used. E.g., "go to /groups/1 to see demo" (which gives me a server error).

The problem with the page crashing might be solved if you just merge main in.

style="background-image:url({member.profile_picture})"
></div>
<p
class=" mt-auto line-clamp-2 max-h-12 text-left text-lg font-bold text-black hover:text-blue-500 md:text-xl"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class=" mt-auto line-clamp-2 max-h-12 text-left text-lg font-bold text-black hover:text-blue-500 md:text-xl"
class="mt-auto line-clamp-2 max-h-12 text-left text-lg font-bold text-black hover:text-blue-500 md:text-xl"

groupAndSortScheduledMeetings,
} from "$lib/utils/summary-helpers";
import LocationIcon from "~icons/material-symbols/location-on";
import ClockIcon from "~icons/material-symbols/nest-clock-farsight-analog-outline";

const sortedMeetings = groupAndSortScheduledMeetings($scheduledMeetings);
export let groupID: number | undefined;
// const modalStore = getModalStore();
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the commented-out code? You can always go back to this PR and check the history if you want to refer to it in the future.

@@ -54,7 +71,7 @@
<RadioItem bind:group={meeting.attending} name="justify" value={"Yes"} required
>Yes</RadioItem
>
<RadioItem bind:group={meeting.attending} name="justify" value={"No"} required
<RadioItem bind:group={meeting.attending} name="justify" value={"No"} required }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<RadioItem bind:group={meeting.attending} name="justify" value={"No"} required }
<RadioItem bind:group={meeting.attending} name="justify" value={"No"} required

@@ -0,0 +1,8 @@
export type Member = {
Copy link
Member

Choose a reason for hiding this comment

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

Should be called GroupMember just to make it clearer.

];

export const members = writable<Member[]>(exampleMembers);
export const attendees = writable<Member[]>(exampleMembers);
Copy link
Member

Choose a reason for hiding this comment

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

This is unused and should be deleted.

@@ -57,6 +57,12 @@ export function groupAndSortScheduledMeetings(scheduledMeetings: ScheduledMeetin
return groupedMeetings;
}

export function filterMeetingsByGroupID(
Copy link
Member

Choose a reason for hiding this comment

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

Please move this (and other relevant function(s)) to the component in which it's used, if it's only used in one.

@MinhxNguyen7
Copy link
Member

We're going to redesign, so I'm going to close this for now. We can pull code from this for the new page when that's ready.

@KevinWu098 KevinWu098 deleted the 22-groups-page branch October 17, 2024 17:57
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.

Groups page
2 participants