-
Notifications
You must be signed in to change notification settings - Fork 7
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 component #8
base: main
Are you sure you want to change the base?
Conversation
creates the main header component and it's subcomponents: NavItem, HeaderSearch, AccountNotification, AccountMenu and HeaderBanner.
Really awesome, one small thing we forgot to say is that the header going to be fixed on top, so it matches with the other component style Other than that looks pretty amazing |
import Image from "next/image"; | ||
import styles from "./account-menu.module.css"; | ||
|
||
const AccountMenu = () => { |
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.
You could use destructuring and create a default value when this data doesn't have filled
Example:
export default (props) => {
const { username = "Kadoodle" } = props
return (
<h1>Hello {username}!</h1>;
)
}
This will ensure that this component could be extensible
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.
@edmolima
just a question, if the solution was like this:
export default ({username = "Kadoodle" } : IAccountMenu) => {
return (
<h1>Hello {username}!</h1>;
)
}
Could work?
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.
Hey @guim0, yes this is absolutely work
I told to you use like receive "props" because I use this notation:
export const Example: FC<IExampleProps> = (props) => {
}
But anyway, you can use like this
export default ({username = "Kadoodle" } : IAccountMenu) => {
return (
<h1>Hello {username}!</h1>;
)
}
I mean, do not use just export default anonymous function, is prefere to use export default NameComponent
this should be work better
|
||
background: none; | ||
|
||
border: 1px solid #0F121D; |
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.
There is also a possibility of leaving all these hexadecimal values in variables
if it is not necessary to separate it into a file, it makes sense to leave it at the top of the file
.search:focus { | ||
color: #676B7D; | ||
background-color: #FFFFFF; | ||
} | ||
|
||
.search::-webkit-input-placeholder { /* WebKit, Blink, Edge */ | ||
color:#A6ACCA; | ||
} | ||
.search:-moz-placeholder { /* Mozilla Firefox 4 to 18 */ | ||
color:#A6ACCA; | ||
opacity: 1; | ||
} | ||
.search::-moz-placeholder { /* Mozilla Firefox 19+ */ | ||
color:#A6ACCA; | ||
opacity: 1; | ||
} | ||
.search:-ms-input-placeholder { /* Internet Explorer 10-11 */ | ||
color:#A6ACCA; | ||
} | ||
.search::-ms-input-placeholder { /* Microsoft Edge */ | ||
color:#A6ACCA; | ||
} | ||
.search::placeholder { /* Most modern browsers support this now. */ | ||
color:#A6ACCA; | ||
} |
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.
I also think we should use variables instead of manually have the hexadecimal for each color definition
<a href={href} target={target}> | ||
<li className={isActive ? styles.active : styles.default}>{label}</li> | ||
</a> |
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.
We can have <li><a></a></li>
if we wanna follow the best HTML semantics practice. See https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/Document_and_website_structure.
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.
you are absolutely right, going to fix it.
<div className={[styles.banner, styles.warning].join(" ")} onClick={onClick}> | ||
<AlertTriangle className={styles.icon} /><a href={href} target="_blank">{message} Click here to know more.</a> | ||
</div> |
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.
We should avoid using onClick
on <div>
, instead a <button>
would be a better option for this case. See https://www.w3schools.com/accessibility/accessibility_buttons_links.php and https://www.yanandcoffee.com/2020/08/10/button-vs-div/ if you wanna understand how to still use a <div>
but with a button role.
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.
great point, i'm going to add role="button"
to this div since it's not really a button. thanks!
<span className={styles.greeting}> | ||
<span className={styles.username}>{username}</span> | ||
</span> |
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.
why having a inside a ? could this be just one with the 2 classes?
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.
I think the purpose is to give a difference to the username text but add the "Hey, "
this could solve it:
on the component:
<p className={styles.greeting}>
Hey, <span>{username}</span>
</p>
on CSS:
.greeting {
font: 16px;
font-weight: 400;
color: #717F9E;
> span {
font-weight: 600;
color: #FFFFFF;
}
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.
I see!! I missed that content on CSS.
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.
I think the purpose is to give a difference to the username text but add the "Hey, " this could solve it:
on the component:
<p className={styles.greeting}> Hey, <span>{username}</span> </p>
on CSS:
.greeting { font: 16px; font-weight: 400; color: #717F9E; > span { font-weight: 600; color: #FFFFFF; }
that's it, the greeting and username have different colors and font-weight, thus explaining the nested spans but your solution has better html semantic. going to improve it. thanks!
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.
LGTM
src/assets/Icons/MagnifyingGlass.svg
Outdated
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.
recomendo utilizar a lib https://lucide.dev/icons/search
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.
sim verdade, salvei esse svg antes de ver a lib e esqueci de apagar depois, vou corrigir.
|
||
const usernamePlaceholder = "Kadoodle" | ||
|
||
const AccountMenu: React.FC<Props> = ({ username = usernamePlaceholder, avatar = AvatarPlaceholder }) => { |
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.
eu removeria esses campos default e passaria eles na chamada desse componente
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.
coloquei esses valores default pra mockar o username e o avatar e depois remover quando for feita a integração com o serviço, mas realmente não faz muito sentido adicionar algo pra remover depois. vou fazer essa correção.
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.
minha sugestão seria juntar tudo em um único componente HeaderBanner
e passar uma prop type: 'warning' | 'announcement' | 'live'
import { AlertTriangle, LucideIcon, Megaphone } from "lucide-react";
import styles from "./style.module.css";
import { Twitch } from "@/assets/Icons";
type BannerType = "warning" | "announcement" | "live";
type Props = {
type: BannerType;
href?: string;
message: string;
onClick?: () => void;
};
const bannerTypes: Record<BannerType, {icon: LucideIcon, style: string}> = {
announcement: {
icon: Megaphone,
style: styles.announcement
},
live: {
icon: Twitch,
style: styles.live
},
warning: {
icon: AlertTriangle,
style: styles.warning
}
}
const HeaderBanner: React.FC<Props> = ({ type, href, message, onClick }) => {
const {icon: Icon, style} = bannerTypes[type];
return (
<div className={[styles.banner, style].join(" ")} onClick={onClick} role="button">
<Icon className={styles.icon} /><a href={href} target="_blank">{message} Click here to know more.</a>
</div>
);
};
algo nesse sentido
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.
excelente sugestão, minha implementação tem muito código repetido, vou refazer esse componente.
removes repeated code, facilitates adition of new banner types and fixes minor positioning issue
Sumary
Creates the header component with flexibility for future changes and/or additions. The additions include the main header, its two controlled states (Guest and Logged In), and its subcomponents with mocked values.
Changes
src/components/Header/index.tsx
: Creates the main Header component. The Logged In/Guest state is controlled by the "isLogged" constant which determines wheter<LoggedInComponents/>
is rendered or not.src/components/Header/HeaderSearch/index.tsx
: Creates the HeaderSearch component. This is a controlled input that is controlled in the parent component by thehandleChange
function and activated by pressing theEnter
key.src/components/Header/HeaderNavItem/index.tsx
: Creates the NavItem component that has three styles: default, hover and active. The "isActive" constant checks if the NavItem href is the current page, if true, it makes the NavItem active.src/components/Header/AccountNotification/index.tsx
: Creates the AccountNotification component. This component simply renders an icon that can bet set through props and the number of notifications.src/components/Header/AccountMenu/index.tsx
: Creates the AccountMenu component. This component shows the username and avatar of the logged in user.src/components/Header/HeaderBanner/index.tsx
: Creates three banner components for warnings, announcements and twitch lives notifications. Each banner have a "message", href and "onClick" prop for different uses.Images
Guest header:
Logged In header:
Header with banners:
Behavior: