Skip to content

Navbar#13

Open
IshikaMeghaSaha wants to merge 4 commits into
mainfrom
Navbar
Open

Navbar#13
IshikaMeghaSaha wants to merge 4 commits into
mainfrom
Navbar

Conversation

@IshikaMeghaSaha

Copy link
Copy Markdown

Issue #7

@FaizBShah FaizBShah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can delete the Navbar.types.ts file as its not being used. Also, a lot of styling issues is occuring due to the ul elements. Also, don't use float in css for styling. Try to use flexbox or css grid instead.

import { NavbarProps as Props } from "./Navbar.types";
import styles from "./styles/Navbar.module.scss";
import { FC } from 'react'
import { NavbarProps as Props } from "./Navbar.types"

@FaizBShah FaizBShah Apr 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this line: import { NavbarProps as Props } from "./Navbar.types" as we are not using any props for this component

import { NavbarProps as Props } from "./Navbar.types"
import styles from "./styles/Navbar.module.scss"

const Navbar: FC<Props> = ({}) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const Navbar: FC<Props> = ({}) => {
const Navbar: FC = () => {

Change it like this as suggested

return (
<div>
<div className={styles.navbar}>
<ul>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use <ul> here. Its used when you when want to list items. Also <ul> can only have <li> as child elements. So you cannot add img or another ul inside it. Use a div instead.

<div className={styles.navbar}>
<ul>
<img className={styles.logo} src="public/images/logo.svg"></img>
<ul className={styles.left}>Squeryll</ul>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above. Use div instead.

<div>
<div className={styles.navbar}>
<ul>
<img className={styles.logo} src="public/images/logo.svg"></img>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<img className={styles.logo} src="public/images/logo.svg"></img>
<img className={styles.logo} src="images/logo.svg"></img>

Comment thread pages/index.tsx
Comment on lines +4 to +6
<div>
<Navbar/>
</div>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<div>
<Navbar/>
</div>
<>
<Navbar/>
</>

That div was just adding an unnecessary element in the DOM. Use Fragments instead for such cases.

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.

2 participants