-
Notifications
You must be signed in to change notification settings - Fork 0
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 header, categoryBar, and categoryButton components #9
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest a few minor changes and discussion points
overflow-x-auto bg-[#1b192e] border-b | ||
border-gray-700 sm:gap-6 sm:px-6`; | ||
|
||
const CategoryBar: React.FC = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const CategoryBar: React.FC = () => { | |
const CategoryBar = () => { |
I believe React.FC types are no longer required (meaning you don't need to import React either)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gurtatiLND can you make a decision to commit this or not commit with a comment explaining why please :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry a few more changes, I think there are a few instances of redundant comments, I've marked them up and you can commit them directly from github if you agree. Also there is a merge conflict that needs to be resolved, as soon as those are all addressed I can approve and merge! :)
overflow-x-auto bg-[#1b192e] border-b | ||
border-gray-700 sm:gap-6 sm:px-6`; | ||
|
||
const CategoryBar: React.FC = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gurtatiLND can you make a decision to commit this or not commit with a comment explaining why please :)
// Toggle category selection | ||
if (category === 'All') { | ||
setSelectedCategories(['All']); // Reset to "All" | ||
} else { | ||
setSelectedCategories( | ||
(prev) => | ||
prev.includes(category) | ||
? prev.filter((c) => c !== category) // Remove if already selected | ||
: [...prev.filter((c) => c !== 'All'), category] // Add new category, deselect "All" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these comments? Suggest removing as they seem redundant
// Toggle category selection | |
if (category === 'All') { | |
setSelectedCategories(['All']); // Reset to "All" | |
} else { | |
setSelectedCategories( | |
(prev) => | |
prev.includes(category) | |
? prev.filter((c) => c !== category) // Remove if already selected | |
: [...prev.filter((c) => c !== 'All'), category] // Add new category, deselect "All" | |
if (category === 'All') { | |
setSelectedCategories(['All']); | |
} else { | |
setSelectedCategories( | |
(prev) => | |
prev.includes(category) | |
? prev.filter((c) => c !== category) | |
: [...prev.filter((c) => c !== 'All'), category] |
Things We Do | ||
</h1> | ||
<Button | ||
label="?" // The button text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label="?" // The button text | |
label="?" |
redundant comment
</div> | ||
|
||
{/* Right side: Image */} | ||
<div className="relative w-10 h-10 sm:w-12 sm:h-12"> {/* Explicit dimensions */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div className="relative w-10 h-10 sm:w-12 sm:h-12"> {/* Explicit dimensions */} | |
<div className="relative w-10 h-10 sm:w-12 sm:h-12"> |
redundant comment
src="/icons/dummy_img.png" | ||
alt="Logo" | ||
sizes="(max-width: 768px) 100vw, 50vw" | ||
rounded={false} // Set to true for rounded images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rounded={false} // Set to true for rounded images | |
rounded={false} |
redundant comment (the parameter is already called "rounded")
label: string; // The text displayed on the button | ||
onClick?: () => void; // Optional: Click handler | ||
className?: string; // Optional: Additional Tailwind or custom classes | ||
ariaPressed?: boolean; // Optional: For accessibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label: string; // The text displayed on the button | |
onClick?: () => void; // Optional: Click handler | |
className?: string; // Optional: Additional Tailwind or custom classes | |
ariaPressed?: boolean; // Optional: For accessibility | |
label: string; | |
onClick?: () => void; | |
className?: string; | |
ariaPressed?: boolean; |
redundant comments - these parameters are well named and self-explanatory :)
fill // Makes the image responsive | ||
sizes={sizes} | ||
style={{ objectFit: 'cover' }} // Ensures proper scaling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fill // Makes the image responsive | |
sizes={sizes} | |
style={{ objectFit: 'cover' }} // Ensures proper scaling | |
fill | |
sizes={sizes} | |
style={{ objectFit: 'cover' }} |
possibly redundant comments
What type of PR is this? (check all applicable)
Description
1. Header Component
Header
component with:<h1>
) and a question mark button on the left (reserved for future functionality).ImageComponent
into theHeader
.next/image
API.fill
behavior.2. Styling Improvements
3. Create and then Delete CategoryButton Component
4. CategoryBar Component
CategoryBar
component that uses theButton
component to render a list of category buttons dynamically and now they can be chosen5. Global CSS Updates
header
class inglobal.css
with:background
: Defines the header background styling.border-bottom
: Adds a bottom border to the header for visual separation.6. Create ImageComponent:
next/image
API:objectFit
withstyle={{ objectFit: 'cover' }}
.sizes
prop to enhance image loading efficiency.rounded
prop.Screenshots
UI accessibility checklist
If your PR includes UI changes, please utilize this checklist:
Critical
andSerious
issues?Added/updated tests?
Please aim to keep the code coverage percentage at 80% and above.
What gif best describes this PR or how it makes you feel?