Skip to content

Feat/clubs #284

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

Open
wants to merge 21 commits into
base: staged
Choose a base branch
from
Open

Feat/clubs #284

wants to merge 21 commits into from

Conversation

Aryawart-kathpal
Copy link
Member

Updted ui for single clubs page

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

server/db/schema/notifications.schema.ts:26

  • The commented-out clubId field suggests a missing foreign key relation in notifications; if not needed, consider removing it to avoid confusion.
// clubId: integer('club_id').references(() => clubs.id),

server/db/schema/clubs.schema.ts:43

  • Consider removing or updating the commented-out code for facultyInchargeId3 if it is no longer intended for use to improve code clarity.
// facultyInchargeId3: integer('faculty_incharge_id3').references(() => faculty.id

server/db/schema/events.ts:16

  • [nitpick] Consider using a more descriptive column name (e.g. 'start_date') instead of 'date' for the startDate field to improve clarity and consistency.
startDate: timestamp('date').notNull(),

app/[locale]/@modals/(.)student-activities/clubs/[dispaly_name]/event/page.tsx:1

  • The folder parameter '[dispaly_name]' appears to be misspelled; consider renaming it to '[display_name]' for consistency with other parts of the codebase.
import Image from 'next/image';

Copy link
Member

Choose a reason for hiding this comment

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

why modify this file

Copy link
Member

Choose a reason for hiding this comment

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

don't edit components unless you have a good reason to.

@aryansri-19
Copy link
Contributor

aryansri-19 commented Apr 14, 2025

Why are the changes commented out?
Also, don't make additional schema file for club Events, use the existing events.schema.ts and add necessary fields related to clubs such as clubId (nullable) etc. The category for all club events would be student so it fits with our current events schema.

@Aryawart-kathpal
Copy link
Member Author

@aryansri-19 please review these schema changes
Deployement will fail for now, migration has to be created

Copy link
Contributor

@aryansri-19 aryansri-19 left a comment

Choose a reason for hiding this comment

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

modify the faculty schema too for the many relation to clubHeads

@aryansri-19
Copy link
Contributor

and rebase once.

@@ -42,9 +44,10 @@ export default function ImageHeader({
{title}
</h1>
)}
{children}
Copy link
Contributor

Choose a reason for hiding this comment

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

out of scope of the pr

Copy link
Member Author

Choose a reason for hiding this comment

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

These changes were not made by me but priyanshu sir, I just reverted to this state.

Copy link
Contributor

Choose a reason for hiding this comment

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

What?, when?

Copy link
Member Author

Choose a reason for hiding this comment

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

{children} is required for the text(heading and logo) that is displayed on the top of banner.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm ok, just make that a separate commit when you are cleaning the tree, also why not just have a prop as image url or something similar, I think this is the only way we need something like this

Copy link
Member Author

Choose a reason for hiding this comment

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

In my latest commit, I have made this change, review this too..
I have added the display_name and logoUrl as props and removed the {chilren} prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Par yaar goal nahi mara

@Aryawart-kathpal
Copy link
Member Author

@aryansri-19 please review the schema changes

@heydoyouknowme0
Copy link
Contributor

base your pr on master or another pr if its a stacked pr

Copy link
Contributor

@aryansri-19 aryansri-19 left a comment

Choose a reason for hiding this comment

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

😢

})
.notNull(),
createdAt: t.timestamp().defaultNow().notNull(),
updatedAt: t
.timestamp()
.$onUpdate(() => new Date())
.notNull(),
clubId: integer('club_id').references(() => clubs.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

t.integer() callback method should be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be changed semantically. Although its modal is the one that will always open, yet it should at least render the correct page.

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.

5 participants