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

Header component (fixes #9) #227

Merged
merged 8 commits into from
Mar 18, 2020

Conversation

stanislawK
Copy link
Contributor

Run app.
Go to http://localhost:8080/.
Navbar should appear on top.
Try to click elements of navbar, and check if they navigate to correct page elements.
Check mobile view - navar should be vertical

@w1stler w1stler mentioned this pull request Jan 8, 2020
@Bosmanfrx
Copy link
Contributor

Just some nitpicking, but generally LGTM

Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

Otherwise I'm cool, but the navbar doesn't seem to be coded for mobile view so most of the comments apply to that problem.

frontend/src/components/Header.vue Outdated Show resolved Hide resolved
frontend/src/components/Header.vue Outdated Show resolved Hide resolved
frontend/src/components/Header.vue Show resolved Hide resolved
frontend/src/components/Header.vue Show resolved Hide resolved
frontend/src/components/Header.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

Some questions not necessarily for this PR

background-color="transparent"
right
class="hidden-sm-and-down"
mobile-break-point="768px"
Copy link
Contributor

Choose a reason for hiding this comment

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

When I saw that I first thought that we've already have defined breakpoints in scss/breakpoints.scss but it didn't work when I exchangaged "768px" for "$phone" variable. Then @Bosmanfrx enlightened me that usually there are two sets of mirrored breakpoints one for css and one for js, which made me realize it is sort of obvious why css variable doesn't make js do what I want. But still having two sets of breakpoints seems not necessary so I looked for the ways of using css breakpoint in js.

Here's interesting solution I found
https://gomakethings.com/the-easy-way-to-manage-css-breakpoints-in-javascript/

As noted, this is beyond this PR but we could think of applying this solution for some other task?
EDIT: after talking with @stanislawK we can also use vuetify grid and e.g. and import its breakpoint values in scss as (at least as far as I'm concerned) the explicit definitions are needed in scss to code media query properly

frontend/src/components/Header.vue Show resolved Hide resolved
frontend/src/components/Header.vue Show resolved Hide resolved
frontend/src/components/Header.vue Show resolved Hide resolved
frontend/src/components/Header.vue Outdated Show resolved Hide resolved
frontend/src/components/Header.vue Outdated Show resolved Hide resolved
@OtisRed
Copy link
Contributor

OtisRed commented Mar 11, 2020

@MargoWM @w1stler @Loczi94 @Bosmanfrx @magul see the comments above. In your aesthetic opinion the items in mobile view should be centered (the imageor adjusted to the left?

To the left:
Zrzut ekranu z 2020-03-11 22-00-39

Centered (there's a slight lean to the left in this picture but it's been fixed now):
Zrzut ekranu z 2020-03-11 20-44-38

Please merge after satysfying conclusion

@MargoWM
Copy link
Contributor

MargoWM commented Mar 13, 2020

For me, the better option is the second one. I prefer symmetry.

@OtisRed OtisRed merged commit d4b500a into CodeForPoznan:master Mar 18, 2020
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.

4 participants