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

New landing page #15

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

New landing page #15

wants to merge 8 commits into from

Conversation

ddebixx
Copy link
Member

@ddebixx ddebixx commented Dec 23, 2022

Check please, buziaki <3

@vercel
Copy link

vercel bot commented Dec 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
sanavaulth ✅ Ready (Inspect) Visit Preview Dec 28, 2022 at 9:29AM (UTC)

Copy link
Member

@konhi konhi left a comment

Choose a reason for hiding this comment

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

few minor changes, but besides, a great job 🔥
you should focus on writting good content though, it's far more important than pretty designs; i.e. lorem ipsum and other fillers suck on fat dick

Comment on lines 24 to 27
max-[768px]:flex-col
max-[768px]:items-center
max-[768px]:justify-even
max-[768px]:h-[300px]">
Copy link
Member

Choose a reason for hiding this comment

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

why not media queries? md: = 768px
https://tailwindcss.com/docs/responsive-design

max-[768px]:justify-even
max-[768px]:h-[300px]">
<p
className="text-[24px]
Copy link
Member

Choose a reason for hiding this comment

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

i think you're overrusing arbitrary values in few places
why not use text-2xl?

Comment on lines +11 to +12
'blackpule': "#000212",
'graypule': "#090B1D",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be purple? XD

pages/index.tsx Outdated
import Navbar from "features/landing-page/Navbar";
import Main from "features/landing-page/Main";
import Footer from "features/landing-page/Footer";
import { ClassNames } from "@emotion/react";
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary import

pages/index.tsx Outdated

export default function HomePage() {
export default function HomePage(): JSX.Element {
Copy link
Member

Choose a reason for hiding this comment

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

no need to add this return type

package.json Outdated
Comment on lines 15 to 16
"@emotion/react": "^11.10.5",
"@emotion/styled": "^11.10.5",
Copy link
Member

Choose a reason for hiding this comment

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

won't this work without emotion? :( i'd generally encourage to use material icon variable font, because we're adding 3 unnecessary packages just for icons

<button onClick={() => setOpenLogin(false)}>X</button>
</>
: <div>
<button
Copy link
Member

Choose a reason for hiding this comment

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

maybe it makes sense to use Button component? if the styles don't match, create a new variant
this applies to few places in code

Comment on lines 47 to 48
<li className="cursor-pointer">{t("contact")}</li>
<li className="cursor-pointer">{t("team")}</li>
Copy link
Member

Choose a reason for hiding this comment

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

doesn't make sense to add cursor-pointer as class
just make it a link with <Link href="#"
probably applies to few places in code

import { useState } from "react"
import { useTranslation } from "next-i18next";

export default function Navbar() {
Copy link
Member

Choose a reason for hiding this comment

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

  1. default exports are a bad practice and shouldn't be used (nextjs pages are a a exception)
  2. please use const = () => instead of function, it's generally a convention to do so in react codebases

features/landing-page/Navbar.tsx Outdated Show resolved Hide resolved
Co-authored-by: Jan Szymański <hello.konhi@gmail.com>
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.

3 participants