You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2020/08/04 22:00:41 UTC
[GitHub] [camel-website] zregvart commented on a change in pull request #448: Improvize on the style of navbar and navbar items layout
zregvart commented on a change in pull request #448:
URL: https://github.com/apache/camel-website/pull/448#discussion_r465337612
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -35,26 +37,27 @@ body {
}
@media screen and (min-width: 1024px) {
- .navbar-end > .navbar-item,
- .navbar-end .navbar-link {
+ .navbar-end > .navbar-item-section .navbar-item {
color: var(--navbar-font-color);
+ cursor: pointer;
}
- .navbar-end > a.navbar-item:hover,
- .navbar-end .navbar-link:hover {
- color: var(--color-asf-dark-blue);
- }
-
- .navbar-end .navbar-link::after {
- border-color: var(--navbar-font-color);
+ .navbar-end > .navbar-item-section .navbar-item::after {
+ transition: all ease-in-out 0.2s;
+ content: '';
+ position: absolute;
+ height: 2px;
+ width: 0;
+ top: var(--navbar-height);
Review comment:
This is top is positioned relative to [the ancestor](https://developer.mozilla.org/en-US/docs/Web/CSS/position#absolute), not top of the page, so having a `top: var(--navbar-height);` here makes it a bit odd:
I'd remove it, looks better without it:
```suggestion
```
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -35,26 +37,27 @@ body {
}
@media screen and (min-width: 1024px) {
- .navbar-end > .navbar-item,
- .navbar-end .navbar-link {
+ .navbar-end > .navbar-item-section .navbar-item {
color: var(--navbar-font-color);
+ cursor: pointer;
Review comment:
This targets anchor tags, so the default should be `cursor: pointer`, am I missing something?
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
}
- .has-dropdown:not(.is-active) > .navbar-dropdown {
- display: none;
+ .navbar-item-section {
+ width: 47%;
+ height: 10rem;
+ background: var(--color-camel-orange);
+ border-radius: 15px;
+ margin: 0 0.5rem 1.5rem 0;
}
- .has-dropdown:not(.is-active) > .navbar-link::after {
- transform: rotate(-45deg);
+ .navbar-item-section img {
+ display: block;
+ margin-left: calc(50% - 2.5rem);
+ margin-top: 1.5rem;
}
- .has-dropdown.is-active > .navbar-dropdown {
- display: block;
- position: relative;
- border: none;
+ .navbar-item.navbar-topics {
+ text-align: center;
+ color: var(--navbar-background);
+ text-transform: uppercase;
+ font-size: 20px;
}
- .has-dropdown.is-active > .navbar-link::after {
- transform: rotate(135deg);
+ @media screen and (min-width: 480px) and (max-width: 626px) {
+ .navbar-item.navbar-topics {
+ font-size: 18px;
+ }
}
- .navbar-item.navbar-topics,
- .navbar-link.navbar-topics {
- color: var(--navbar-font-color);
+ @media screen and (max-width: 480px) {
+ .navbar-item.navbar-topics {
+ font-size: 15px;
+ }
+ }
+
+ @media screen and (max-width: 375px) {
+ .navbar-item-section {
+ width: 45%;
+ }
+
+ .navbar-item.navbar-topics {
+ font-size: 14px;
Review comment:
We should do `rem`s for `font-size`.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
}
- .has-dropdown:not(.is-active) > .navbar-dropdown {
- display: none;
+ .navbar-item-section {
+ width: 47%;
+ height: 10rem;
+ background: var(--color-camel-orange);
+ border-radius: 15px;
+ margin: 0 0.5rem 1.5rem 0;
}
- .has-dropdown:not(.is-active) > .navbar-link::after {
- transform: rotate(-45deg);
+ .navbar-item-section img {
+ display: block;
+ margin-left: calc(50% - 2.5rem);
+ margin-top: 1.5rem;
}
- .has-dropdown.is-active > .navbar-dropdown {
- display: block;
- position: relative;
- border: none;
+ .navbar-item.navbar-topics {
+ text-align: center;
+ color: var(--navbar-background);
+ text-transform: uppercase;
+ font-size: 20px;
}
- .has-dropdown.is-active > .navbar-link::after {
- transform: rotate(135deg);
+ @media screen and (min-width: 480px) and (max-width: 626px) {
+ .navbar-item.navbar-topics {
+ font-size: 18px;
Review comment:
We should do `rem`s for `font-size`.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
Review comment:
I think this whole `.navbar-divider` rule can be removed, I can't find any elements with `class="navbar-divider"` in our markup.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -63,6 +66,7 @@ body {
display: flex;
flex-grow: 1;
height: var(--navbar-height);
+ outline: 0;
Review comment:
This is targeting a `div` which doesn't have an outline, so it's not needed. Even if it were targeting an element that would be focusable/actionable, we would not want to remove the outline for folk that use keyboard to navigate and see the element that they navigated to via the keyboard.
```suggestion
```
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
}
- .has-dropdown:not(.is-active) > .navbar-dropdown {
- display: none;
+ .navbar-item-section {
+ width: 47%;
+ height: 10rem;
+ background: var(--color-camel-orange);
+ border-radius: 15px;
+ margin: 0 0.5rem 1.5rem 0;
}
- .has-dropdown:not(.is-active) > .navbar-link::after {
- transform: rotate(-45deg);
+ .navbar-item-section img {
+ display: block;
+ margin-left: calc(50% - 2.5rem);
+ margin-top: 1.5rem;
}
- .has-dropdown.is-active > .navbar-dropdown {
- display: block;
- position: relative;
- border: none;
+ .navbar-item.navbar-topics {
+ text-align: center;
+ color: var(--navbar-background);
+ text-transform: uppercase;
+ font-size: 20px;
Review comment:
We should do `rem`s for `font-size`.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
}
- .has-dropdown:not(.is-active) > .navbar-dropdown {
- display: none;
+ .navbar-item-section {
+ width: 47%;
Review comment:
Positioning of this is a bit misaligned:
Perhaps something like:
```diff
@media screen and (max-width: 1023px) {
.navbar-menu.is-active {
- padding: 3rem 1rem 1rem 1.5rem;
+ padding: 1rem;
}
.navbar-item-section {
- width: 47%;
+ width: calc(50% - 1rem);
- margin: 0 .5rem 1.5rem 0;
+ margin: 0.5rem;
}
}
```
##########
File path: antora-ui-camel/src/js/05-navbar.js
##########
@@ -10,11 +11,8 @@ document.addEventListener('DOMContentLoaded', function () {
})
})
- var menuDropDowns = Array.prototype.slice.call(document.querySelectorAll('.has-dropdown'), 0)
- menuDropDowns.forEach(function (el) {
- el.addEventListener('click', function (e) {
- e.stopPropagation()
- el.classList.toggle('is-active')
- })
+ window.addEventListener('scroll', function () {
+ if (this.scrollY <= 30) navbar.classList.remove('scrolled')
Review comment:
This is in pixels, and it need not be 30px depending on the configured user font size or browser font rendering/sizing. I think we should check if an element just below the navbar is clipped or not, [here is an article](https://css-tricks.com/styling-based-on-scroll-position/) on how to do that.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
}
- .has-dropdown:not(.is-active) > .navbar-dropdown {
- display: none;
+ .navbar-item-section {
+ width: 47%;
+ height: 10rem;
+ background: var(--color-camel-orange);
+ border-radius: 15px;
+ margin: 0 0.5rem 1.5rem 0;
}
- .has-dropdown:not(.is-active) > .navbar-link::after {
- transform: rotate(-45deg);
+ .navbar-item-section img {
+ display: block;
+ margin-left: calc(50% - 2.5rem);
+ margin-top: 1.5rem;
}
- .has-dropdown.is-active > .navbar-dropdown {
- display: block;
- position: relative;
- border: none;
+ .navbar-item.navbar-topics {
+ text-align: center;
+ color: var(--navbar-background);
+ text-transform: uppercase;
+ font-size: 20px;
}
- .has-dropdown.is-active > .navbar-link::after {
- transform: rotate(135deg);
+ @media screen and (min-width: 480px) and (max-width: 626px) {
+ .navbar-item.navbar-topics {
+ font-size: 18px;
+ }
}
- .navbar-item.navbar-topics,
- .navbar-link.navbar-topics {
- color: var(--navbar-font-color);
+ @media screen and (max-width: 480px) {
+ .navbar-item.navbar-topics {
+ font-size: 15px;
+ }
+ }
+
+ @media screen and (max-width: 375px) {
+ .navbar-item-section {
+ width: 45%;
+ }
Review comment:
Not sure this is needed.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -174,30 +161,50 @@ body {
top: 8rem;
left: 0;
right: 0;
- padding: 1rem 2rem;
+ padding: 3rem 1rem 1rem 1.5rem;
}
- .has-dropdown:not(.is-active) > .navbar-dropdown {
- display: none;
+ .navbar-item-section {
+ width: 47%;
+ height: 10rem;
+ background: var(--color-camel-orange);
+ border-radius: 15px;
+ margin: 0 0.5rem 1.5rem 0;
}
- .has-dropdown:not(.is-active) > .navbar-link::after {
- transform: rotate(-45deg);
+ .navbar-item-section img {
+ display: block;
+ margin-left: calc(50% - 2.5rem);
+ margin-top: 1.5rem;
}
- .has-dropdown.is-active > .navbar-dropdown {
- display: block;
- position: relative;
- border: none;
+ .navbar-item.navbar-topics {
+ text-align: center;
+ color: var(--navbar-background);
+ text-transform: uppercase;
+ font-size: 20px;
}
- .has-dropdown.is-active > .navbar-link::after {
- transform: rotate(135deg);
+ @media screen and (min-width: 480px) and (max-width: 626px) {
+ .navbar-item.navbar-topics {
+ font-size: 18px;
+ }
}
- .navbar-item.navbar-topics,
- .navbar-link.navbar-topics {
- color: var(--navbar-font-color);
+ @media screen and (max-width: 480px) {
+ .navbar-item.navbar-topics {
+ font-size: 15px;
Review comment:
We should do `rem`s for `font-size`.
##########
File path: antora-ui-camel/src/partials/header-content.hbs
##########
@@ -16,7 +16,10 @@
</div>
</div>
{{else}}
- <a class="navbar-item navbar-topics" href="{{url}}">{{name}}</a>
+ <div class="navbar-item-section">
+ <img src="{{siteRootPath}}/_/img/{{name}}.svg">
+ <a class="navbar-item navbar-topics" href="{{url}}">{{name}}</a>
+ </div>
Review comment:
Why not make this an anchor tag with an image within it, instead of div with image and anchor. Would make the whole item clickable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org