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.

Sign up for GitHub

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

Jump to bottom

Navbar #16

Merged
merged 7 commits into from
Jan 19, 2025
Merged

Navbar #16

merged 7 commits into from
Jan 19, 2025

Conversation

goteusz-maszyk
Copy link
Contributor

@goteusz-maszyk goteusz-maszyk commented Jan 11, 2025 •
edited by Norbiros
Loading

Close #12

@goteusz-maszyk goteusz-maszyk requested a review from a team as a code owner January 11, 2025 22:47
Kubaryt
Kubaryt requested changes Jan 11, 2025
View reviewed changes
Copy link
Member

@Kubaryt Kubaryt left a comment

Choose a reason for hiding this comment

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

Rebase with master, so checks will run

@goteusz-maszyk goteusz-maszyk force-pushed the feat/navbar branch 2 times, most recently from b77b6c5 to b72995f Compare January 11, 2025 22:50
Kubaryt
Kubaryt previously approved these changes Jan 11, 2025
View reviewed changes
@Kubaryt Kubaryt dismissed their stale review January 11, 2025 22:51

i approved only rebase

wikipop
wikipop previously approved these changes Jan 11, 2025
View reviewed changes
Copy link
Contributor

@wikipop wikipop left a comment

Choose a reason for hiding this comment

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

TL;DR LGTM

Kubaryt
Kubaryt previously requested changes Jan 11, 2025
View reviewed changes
Copy link
Member

@Kubaryt Kubaryt left a comment

Choose a reason for hiding this comment

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

Add github link to logo instead of adding separate link on navbar

@goteusz-maszyk
Copy link
Contributor Author

Add github link to logo instead of adding separate link on navbar

no it will be unusable
the main logo is most commonly a '/' link

@Kubaryt Kubaryt requested a review from Norbiros January 11, 2025 23:03
Norbiros
Norbiros requested changes Jan 12, 2025
View reviewed changes
Copy link
Member

@Norbiros Norbiros left a comment

Choose a reason for hiding this comment

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

  1. If you want you could use horizontal navigation component
  2. What about mobile devices?
  3. I'm not sure but I don't think it is centred properly
    image

frontend/app/assets/css/main.css Outdated Show resolved Hide resolved
frontend/public/Quantico-Bold.ttf Outdated Show resolved Hide resolved
frontend/public/logo.svg Outdated Show resolved Hide resolved
frontend/app/assets/css/main.css Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
Norbiros
Norbiros reviewed Jan 12, 2025
View reviewed changes
frontend/app/app.vue Outdated Show resolved Hide resolved
@Norbiros
Copy link
Member

Also, please add ` above navbar, and make sure it looks good.

@Kubaryt Kubaryt dismissed their stale review January 12, 2025 10:28

invalid concern

@goteusz-maszyk
Copy link
Contributor Author

Ja ci dam zaraz not successful u mie przechodziło kurde gópi github jutro się ogarnie

Norbiros
Norbiros requested changes Jan 13, 2025
View reviewed changes
frontend/public/img/logo.svg Outdated Show resolved Hide resolved
frontend/public/Quantico-Bold.ttf Outdated Show resolved Hide resolved
frontend/package.json Outdated Show resolved Hide resolved
frontend/nuxt.config.ts Outdated Show resolved Hide resolved
frontend/app/components/MainNavbar.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/components/MainNavbar.vue Outdated Show resolved Hide resolved
@Norbiros
Copy link
Member

Also you still don't support mobile properly and it doesn't match our figma designs

@goteusz-maszyk goteusz-maszyk force-pushed the feat/navbar branch 4 times, most recently from 5d9d9ec to e0d9414 Compare January 15, 2025 19:48
Norbiros
Norbiros requested changes Jan 15, 2025
View reviewed changes
frontend/app/app.vue Outdated Show resolved Hide resolved
frontend/app/components/Navbar.vue Outdated Show resolved Hide resolved
frontend/app/components/Navbar.vue Outdated Show resolved Hide resolved
frontend/app/components/Navbar.vue Outdated Show resolved Hide resolved
frontend/app/components/Navbar.vue Outdated Show resolved Hide resolved
@Norbiros
Copy link
Member

image
This button looks different on mobile and desktop

@goteusz-maszyk
Copy link
Contributor Author

image This button looks different on mobile and desktop

LMAO you can see that there is a tiny line that nobody cares of or that drop shadow wasn't in one button but you are too blind to see that the buttons are completely different

anyways you are not in the design team so I DONT CARE ABOUT YOUR OPINION

Kubaryt
Kubaryt requested changes Jan 15, 2025
View reviewed changes
frontend/app/components/Navbar.vue Outdated Show resolved Hide resolved
frontend/app/components/Navbar.vue Outdated Show resolved Hide resolved
Kubaryt
Kubaryt previously approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@Kubaryt Kubaryt left a comment

Choose a reason for hiding this comment

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

i trust you that it looks good

Norbiros
Norbiros previously approved these changes Jan 18, 2025
View reviewed changes
@Norbiros Norbiros requested review from wojpo and Kubaryt January 18, 2025 17:43
@wikipop
Copy link
Contributor

wikipop commented Jan 19, 2025

This button is so f ugly i cant 😭😭

wikipop
wikipop requested changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@wikipop wikipop left a comment

Choose a reason for hiding this comment

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

Navbar has way too less margin on mobile viewport

wikipop
wikipop requested changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@wikipop wikipop left a comment

Choose a reason for hiding this comment

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

Navbar elements not properly scaled on mobile viewport (e.g. logo way too big, buttons unclickable)

wojpo
wojpo requested changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@wojpo wojpo left a comment

Choose a reason for hiding this comment

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

navbar on mobile viewport does not meet the design from Figma. (e.g. extendable menu looks awful)

@Norbiros Norbiros requested review from wojpo and wikipop January 19, 2025 15:12
@Norbiros
Copy link
Member

Norbiros commented Jan 19, 2025 •
edited
Loading

Our lead designer approved this design :3

This button is so f ugly i cant 😭😭

Discuss that with the design team

Kubaryt
Kubaryt previously requested changes Jan 19, 2025
View reviewed changes
Copy link
Member

@Kubaryt Kubaryt left a comment

Choose a reason for hiding this comment

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

this logo is too big on mobile version
image

those links on mobile display are too small imo, also they are not centered
image

thats all for now

frontend/app/components/Navbar.vue Show resolved Hide resolved
frontend/app/components/Navbar.vue Show resolved Hide resolved
frontend/app/components/Navbar.vue Show resolved Hide resolved
@Norbiros
Copy link
Member

this logo is too big on mobile version

You are testing older version, we redesigned it

wikipop
wikipop previously approved these changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@wikipop wikipop left a comment

Choose a reason for hiding this comment

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

I mean it still looks plain but Its design team thing ig

@Norbiros Norbiros requested a review from Kubaryt January 19, 2025 16:36
wojpo
wojpo approved these changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@wojpo wojpo left a comment

Choose a reason for hiding this comment

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

APPROVED. Im not a big fan of this navbar but im not in design team so i won't complain. This PR will probably be merged soon so i want to say something: "To make navbar in 31 Cracow highschool you need 5 programmers, 2 designers, work week and 50+ comments on pull request".

@Norbiros Norbiros merged commit 9700638 into master Jan 19, 2025
2 checks passed
@Norbiros Norbiros deleted the feat/navbar branch January 19, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojpo wojpo wojpo approved these changes

@Norbiros Norbiros Norbiros approved these changes

@wikipop wikipop wikipop approved these changes

@Kubaryt Kubaryt Awaiting requested review from Kubaryt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feat]: Create navbar
5 participants
@goteusz-maszyk @Norbiros @wikipop @Kubaryt @wojpo