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 header component #8

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

gllherme
Copy link

@gllherme gllherme commented Feb 4, 2024

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 the handleChange function and activated by pressing the Enter 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:

image

Logged In header:

image

Header with banners:

image

Behavior:

2024-02-04 03-16-14

creates the main header component and it's subcomponents: NavItem, HeaderSearch,
AccountNotification, AccountMenu and HeaderBanner.
@kaduwaengertner
Copy link
Member

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 = () => {
Copy link

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

Copy link

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?

Copy link

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;
Copy link

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

Comment on lines 28 to 52
.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;
}

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

Comment on lines 16 to 18
<a href={href} target={target}>
<li className={isActive ? styles.active : styles.default}>{label}</li>
</a>

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.

Copy link
Author

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.

Comment on lines 14 to 16
<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>

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.

Copy link
Author

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!

Comment on lines 12 to 14
<span className={styles.greeting}>
<span className={styles.username}>{username}</span>
</span>

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?

Copy link

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, "
image
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;
}


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.

Copy link
Author

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, " image 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!

Copy link

@guim0 guim0 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Copy link
Author

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 }) => {

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

Copy link
Author

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.

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

Copy link
Author

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
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.

6 participants