feat(update/director): Update in Directors Message #289
Conversation
| height={0} | ||
| src={imageSrc} | ||
| width={0} |
There was a problem hiding this comment.
The Next/Image component is specified with height and width set to 0, which may prevent proper image rendering. Consider providing appropriate non-zero dimensions or using a layout strategy (like 'fill') to ensure the image displays correctly.
| height={0} | |
| src={imageSrc} | |
| width={0} | |
| layout="intrinsic" | |
| src={imageSrc} |
There was a problem hiding this comment.
This is not faulty. This card component is taken from the faculty page cards.
|
the content inside must not overflow |
|
Do this by keeping image outside the container refer |
aryansri-19
left a comment
aryansri-19
left a comment
There was a problem hiding this comment.
Responsiveness is a little distorted. Fix that.
| <section | ||
| className="container" | ||
| style={{ | ||
| backgroundImage: `linear-gradient(rgba(249, 245, 235, 1) 0%, rgba(249, 245, 235, 0.85) 85%, rgba(249, 245, 235, 1) 100%)`, |
There was a problem hiding this comment.
there is no bg-image behind other officers
|
@som-04 sir, another review please |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Director's Message page by adding new components and constants and enhancing the overall page layout. Key changes include importing new assets and icons, defining constants for the Director and office cards, and introducing a new Card component to unify card presentation across the page.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| package.json | Added new dependency "yarn" to support additional tooling or workflow |
| app/[locale]/director-message/page.tsx | Added constants for director and office cards, incorporated new background images, and implemented a new Card component for displaying director and office information |
| height={0} | ||
| src={imageSrc} | ||
| width={0} |
There was a problem hiding this comment.
Using height={0} (and width={0} on line 255) with next/image may prevent proper image rendering. Consider providing explicit dimensions or using fill if responsive sizing is intended.
| height={0} | |
| src={imageSrc} | |
| width={0} | |
| fill | |
| src={imageSrc} |
| // DIRECTOR's OFFICE | ||
| const directorOfficeCards = [ | ||
| { | ||
| image: `${getS3Url()}/assets/office.jpeg`, |
There was a problem hiding this comment.
[nitpick] The directorOfficeCards array includes multiple entries with identical data. If these are placeholders, please add a comment clarifying; otherwise, consider providing unique details to improve maintainability.
GetPsyched
left a comment
GetPsyched
left a comment
There was a problem hiding this comment.
A few bits of advice on general foot-guns to avoid.
| <article className="w-full"> | ||
| <p className="text-lg max-md:rounded-t md:w-full md:rounded-r "> | ||
| {text.message.slice(0, 7).map((message, index) => ( | ||
| <span key={index} className="mb-5 block"> | ||
| {message} | ||
| </span> | ||
| ))} | ||
| </p> | ||
| </article> |
There was a problem hiding this comment.
This bit is repeated 3 times; I'm assuming this is for responsiveness. Responsiveness should absolutely not warrant writing the same HTML multiple times over, it should only be manipulated using media queries (tailwindcss acting as a wrapper over said media queries here).
HTML defines the structure of what's to be rendered, and CSS defines how to render it.
There can be cases where the design on a certain width looks so wildly different from another width that you can break this rule, but it doesn't look justified here at all.
There was a problem hiding this comment.
Okay, I missed the slice(0, 7) stuff here. This is bad, not your code, but the structure of text.message in the i18n files. @som-04 why was the message a list? Was it to divide in different paragraphs?
There was a problem hiding this comment.
yes, the message is divided into paragraphs so each para is a list element, was done to avoid any repetitive lines.
There was a problem hiding this comment.
Which other page is this used in? Because here it's being spliced into 3 blocks instead of like 10 that the i18n list has
There was a problem hiding this comment.
oh my bad, even lines containing words like 'Jai Hind' and Director's name were added to message array, so certain indexes were accessed separately in other lines.
Also, the formatting of the message wasn't entirely uniform in the old website (the design followed at that time), so 3 blocks were used; as the 2nd block, The logo of NIT KKR, has a Motto which reads as follows... “Shramaye Anavarat chesta cha” is centered.
This pull request includes significant updates to the
app/[locale]/director-message/page.tsxfile, focusing on adding new components and improving the page layout for the Director's message and office information.Key Changes:
New Components and Constants:
Image,FaPhone, andMdEmailto enhance the visual and functional aspects of the page. (app/[locale]/director-message/page.tsxR1-R65)Page Layout Enhancements:
DirectorCornerfunction to include sections for the Director's card and office cards, each with its own styling and layout. (app/[locale]/director-message/page.tsxR74-R159)New Card Component:
Cardcomponent to encapsulate the layout and styling of individual cards for the Director and office members. This component includes props for image, name, designation, and contact details, and ensures a consistent presentation across the page. (app/[locale]/director-message/page.tsxR212-R277)