zregvart commented on a change in pull request #398:
URL: https://github.com/apache/camel-website/pull/398#discussion_r445436538
##########
File path: antora-ui-camel/src/css/doc.css
##########
@@ -532,6 +538,19 @@
padding: 0.75rem;
}
+.doc aside {
+ float: right;
+ margin-left: 1rem;
+ width: 25rem;
+ margin-top: 3.5rem;
+}
+
+@media screen and (max-width: 1023px) {
+ .doc.side aside {
+ margin-left: -1rem;
Review comment:
That sounds like a workaround, I'd prefer that we don't implement
workarounds, they tend to get back at us.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -59,18 +65,26 @@ body {
height: var(--navbar-height);
}
+@media screen and (max-width: 1277px) {
+ .navbar-brand {
+ flex-wrap: wrap;
+ }
+}
+
.navbar-burger {
color: var(--navbar-font-color);
- background: none;
+ background: var(--navbar-background);
border: none;
outline: none;
line-height: 1;
- height: var(--navbar-height);
+ width: 2rem;
+ height: 2rem;
+ top: 1.25rem;
+ float: right;
Review comment:
Not sure if we want to use float and flex on the same element, do we
need this?
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -417,18 +445,18 @@ body {
}
}
-#search-cancel {
+.search-cancel {
position: relative;
bottom: calc(50% - 0.15rem);
- left: calc(100% - 1.25rem);
+ left: calc(100% - 1.5rem);
height: 1rem;
display: none;
cursor: pointer;
}
-#search_results {
+.search-results {
Review comment:
Now we have only one element, should we target by id instead?
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -28,7 +34,7 @@ body {
padding: 0 0.375rem;
}
-@media screen and (min-width: 1024px) {
+@media screen and (min-width: 1278px) {
Review comment:
I wonder if we should wait for #397 to get merged so we don't need to
change this breakpoint
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -59,18 +65,26 @@ body {
height: var(--navbar-height);
}
+@media screen and (max-width: 1277px) {
+ .navbar-brand {
+ flex-wrap: wrap;
Review comment:
I think this is causing some issues on Firefox, oddly not in the
responsive tool but when the browser is resized. Not sure if we can prevent
this.
When the browser is resized:
In responsive tool:
##########
File path: antora-ui-camel/src/css/doc.css
##########
@@ -532,6 +538,19 @@
padding: 0.75rem;
}
+.doc aside {
+ float: right;
Review comment:
I don think this `float` is needed here
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -59,18 +65,26 @@ body {
height: var(--navbar-height);
}
+@media screen and (max-width: 1277px) {
+ .navbar-brand {
+ flex-wrap: wrap;
+ }
+}
+
.navbar-burger {
color: var(--navbar-font-color);
- background: none;
+ background: var(--navbar-background);
border: none;
outline: none;
line-height: 1;
- height: var(--navbar-height);
Review comment:
Could we use `var(--navbar-height)` instead of `2rem` below?
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -59,18 +65,26 @@ body {
height: var(--navbar-height);
}
+@media screen and (max-width: 1277px) {
+ .navbar-brand {
+ flex-wrap: wrap;
+ }
+}
+
.navbar-burger {
color: var(--navbar-font-color);
- background: none;
+ background: var(--navbar-background);
Review comment:
Not sure if this is needed, doesn't seem to make a difference. Perhaps
I'm missing something.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -443,19 +471,19 @@ body {
padding-right: 0.5rem;
}
-#search_results dt {
+.search-results dt {
padding: 0.5rem;
color: var(--color-camel-orange);
text-transform: uppercase;
border-top: 1px solid var(--navbar-search-result-separator);
border-bottom: 1px solid var(--navbar-search-result-separator);
}
-#search_results a {
+.search-results a {
Review comment:
Now we have only one element, should we target by id instead?
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -80,20 +94,20 @@ body {
.navbar-burger span {
background: var(--navbar-font-color);
display: block;
- height: 1px;
+ height: 2px;
Review comment:
Thinner looked more elegant to me, not sure if we need to make it
thicker.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -417,18 +445,18 @@ body {
}
}
-#search-cancel {
+.search-cancel {
Review comment:
Now we have only one element, should we target by id instead?
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -156,14 +172,19 @@ body {
margin: 0.25rem 0;
}
-@media screen and (max-width: 1023px) and (min-width: 480px) {
+@media screen and (max-width: 1277px) {
+ .navbar-menu.is-active {
+ top: 7.75rem;
Review comment:
I think we should derive this from the navbar height, I can see that
this will go out of alignment if we change the navbar height variable. Perhaps
using the variable here or calculating the hight would be better.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -443,19 +471,19 @@ body {
padding-right: 0.5rem;
}
-#search_results dt {
+.search-results dt {
Review comment:
Now we have only one element, should we target by id instead?
##########
File path: antora-ui-camel/src/css/nav.css
##########
@@ -24,13 +24,19 @@
}
}
+@media screen and (max-width: 1023px) {
+ .nav-container {
+ top: 7.75rem;
Review comment:
Perhaps derive a value using `calc` or use a variable here.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -475,7 +503,7 @@ body {
color: var(--color-jet-50);
}
-.results-hidden #search_results {
+.results-hidden .search-results {
Review comment:
Now we have only one element, should we target by id instead?
##########
File path: antora-ui-camel/src/css/nav.css
##########
@@ -44,10 +50,25 @@
@media screen and (min-width: 1024px) {
.nav {
top: var(--navbar-height);
+ height: var(--nav-menu-panel-height);
+ }
+
+ .nav-category {
+ top: 8.5rem;
box-shadow: none;
position: sticky;
height: var(--nav-height--desktop);
}
+
+ .nav-components {
+ top: 6.75rem;
Review comment:
Perhaps derive a value using calc or use a variable here.
##########
File path: antora-ui-camel/src/css/nav.css
##########
@@ -118,6 +152,12 @@ html.is-clipped--nav {
overflow-y: auto;
}
+@media screen and (max-width: 1277px) {
+ .nav-menu {
+ top: 3.5rem;
Review comment:
Perhaps derive a value using calc or use a variable here.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -387,11 +407,19 @@ body {
.navbar-fill {
flex-grow: 1;
+ order: 3;
+}
+
+.break-row {
+ display: none;
+ flex-basis: 100%;
+ height: 0;
Review comment:
Do we need `height: 0` with `display: none`?
##########
File path: antora-ui-camel/src/css/nav.css
##########
@@ -44,10 +50,25 @@
@media screen and (min-width: 1024px) {
.nav {
top: var(--navbar-height);
+ height: var(--nav-menu-panel-height);
+ }
+
+ .nav-category {
+ top: 8.5rem;
Review comment:
Perhaps derive a value using calc or use a variable here.
##########
File path: antora-ui-camel/src/css/header.css
##########
@@ -443,19 +471,19 @@ body {
padding-right: 0.5rem;
}
-#search_results dt {
+.search-results dt {
padding: 0.5rem;
color: var(--color-camel-orange);
text-transform: uppercase;
border-top: 1px solid var(--navbar-search-result-separator);
border-bottom: 1px solid var(--navbar-search-result-separator);
}
-#search_results a {
+.search-results a {
padding: 0.5rem;
}
-#search_results a:hover {
+.search-results a:hover {
Review comment:
Now we have only one element, should we target by id instead?
##########
File path: antora-ui-camel/src/partials/header-content.hbs
##########
@@ -6,32 +6,39 @@
<div class="navbar-end">
{{#withMenuData}}
{{#each @items}}
- {{#if children}}
- <div class="navbar-item has-dropdown is-hoverable">
+ {{#if children}}
+ <div class="navbar-item has-dropdown is-hoverable">
<a class="navbar-link navbar-topics" href="#">{{name}}</a>
<div class="navbar-dropdown">
{{#each children}}
<a class="navbar-item" href="{{url}}">{{name}}</a>
{{/each}}
</div>
- </div>
- {{else}}
- <a class="navbar-item navbar-topics"
href="{{url}}">{{name}}</a>
- {{/if}}
+ </div>
+ {{else}}
+ <a class="navbar-item navbar-topics" href="{{url}}">{{name}}</a>
+ {{/if}}
{{/each}}
{{/withMenuData}}
</div>
</div>
<div class="navbar-fill"></div>
+ <div class="break-row"></div>
<div class="navbar-search results-hidden">
<input id="search" class="search" placeholder="Search"
autocomplete="off">
- <img src= "/_/img/cancel.svg" alt="Clear" id="search-cancel">
- <div id="search_results"></div>
+ <img src= "/_/img/cancel.svg" alt="Clear" id="search-cancel"
class="search-cancel">
+ <div id="search_results" class="search-results"></div>
Review comment:
Do we need the `search-results` class here?
##########
File path: antora-ui-camel/src/js/vendor/algoliasearch.bundle.js
##########
@@ -6,10 +6,12 @@
window.addEventListener('load', () => {
const client = algoliasearch('BH4D9OD16A',
'16e3a9155a136e4962dc4c206f8278bd')
const index = client.initIndex('apache_camel')
- const search = document.querySelector('#search')
+ const search = document.querySelector('.search')
const container = search.parentNode
- const results = document.querySelector('#search_results')
- const cancel = document.querySelector('#search-cancel')
+ const results = document.querySelector('.search-results')
+ const cancel = document.querySelector('.search-cancel')
+ const className = 'navbar-search'
+ const hiddenClass = 'navbar-search results-hidden'
Review comment:
Do we still need these changes to the JS code now that we have only one
search input?
##########
File path: antora-ui-camel/src/partials/header-content.hbs
##########
@@ -6,32 +6,39 @@
<div class="navbar-end">
{{#withMenuData}}
{{#each @items}}
- {{#if children}}
- <div class="navbar-item has-dropdown is-hoverable">
+ {{#if children}}
+ <div class="navbar-item has-dropdown is-hoverable">
<a class="navbar-link navbar-topics" href="#">{{name}}</a>
<div class="navbar-dropdown">
{{#each children}}
<a class="navbar-item" href="{{url}}">{{name}}</a>
{{/each}}
</div>
- </div>
- {{else}}
- <a class="navbar-item navbar-topics"
href="{{url}}">{{name}}</a>
- {{/if}}
+ </div>
+ {{else}}
+ <a class="navbar-item navbar-topics" href="{{url}}">{{name}}</a>
+ {{/if}}
{{/each}}
{{/withMenuData}}
</div>
</div>
<div class="navbar-fill"></div>
+ <div class="break-row"></div>
<div class="navbar-search results-hidden">
<input id="search" class="search" placeholder="Search"
autocomplete="off">
- <img src= "/_/img/cancel.svg" alt="Clear" id="search-cancel">
- <div id="search_results"></div>
+ <img src= "/_/img/cancel.svg" alt="Clear" id="search-cancel"
class="search-cancel">
Review comment:
Do we need the `search-cancel` class here?
##########
File path: antora-ui-camel/src/partials/nav.hbs
##########
@@ -1,5 +1,9 @@
<div class="nav-container"{{#if page.component}}
data-component="{{page.component.name}}" data-version="{{page.version}}"{{/if}}>
+ {{#if (eq page.component.name 'components')}}
+ <aside class="nav nav-components">
+ {{else}}
<aside class="nav">
+ {{/if}}
Review comment:
I think this could be simpler, all `aside.nav` should cater to the same
positioning, not sure why components would be different, perhaps the parent of
this aside (`.nav-container`) should be positioned instead.
----------------------------------------------------------------
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:
us...@infra.apache.org
