[GitHub] [camel-website] zregvart commented on a change in pull request #536: chore: add initial version of catalog layout

[GitBox <gi...@apache.org>]

zregvart commented on a change in pull request #536:
URL: https://github.com/apache/camel-website/pull/536#discussion_r574452105



##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {

Review comment:
       We prefix all content categories with a class to minimize the chance of rule collision and increase the specificity of the rule. With that it would be better to use `.catalog .container` selector.
   
   ```suggestion
   .catalog .container {
   ```

##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {
+  margin-top: 20px;

Review comment:
       Always use `rem`, this is to help with different display devices and font customization in end user agents. 20px should be ~ 1.2rem (a bit more).
   
   ```suggestion
     margin-top: 1.2rem;
   ```

##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {
+  margin-top: 20px;
+  display: flex;
+  flex-wrap: wrap;
+}
+
+.catalog-entry {
+  width: 250px;
+  min-height: 140px;
+  padding: 15px;
+  margin: 5px;
+  border-style: solid;
+  border-color: #eee;
+  border-width: 2px;
+  text-align: center;
+}
+
+.catalog-entry .image {

Review comment:
       Perhaps something like this would be more descriptive:
   
   ```suggestion
   .catalog .entry .icon {
   ```

##########
File path: antora-ui-camel/src/partials/catalog.hbs
##########
@@ -0,0 +1,20 @@
+{{#if navigation.length}}
+  {{#each navigation}}
+    {{#if ../show}}{{#if ./content}}
+      <div class="catalog-entry">
+        <a href="{{~#if (eq ./urlType 'internal')}}{{{relativize ./url}}}{{~else}}{{{./url}}}{{~/if}}">

Review comment:
       Not sure about the purpose of this, we enforce all links to be relative via configuration and a HTML check.

##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {
+  margin-top: 20px;
+  display: flex;
+  flex-wrap: wrap;
+}
+
+.catalog-entry {
+  width: 250px;
+  min-height: 140px;
+  padding: 15px;
+  margin: 5px;
+  border-style: solid;
+  border-color: #eee;
+  border-width: 2px;

Review comment:
       Use variables, `--color-smoke-90` is near, not sure about `2px` (borders are just about the only place where `px` makes sense):
   
   ```suggestion
     border: 2px solid var(--color-smoke-90);
   ```

##########
File path: antora-ui-camel/src/partials/catalog-article.hbs
##########
@@ -0,0 +1,12 @@
+<article class="doc">
+{{#with page.title}}
+<h1 class="page">{{{this}}}</h1>
+{{/with}}
+{{{page.contents}}}
+<div class="catalog-container">

Review comment:
       ```suggestion
   <div class="container">
   ```

##########
File path: antora-ui-camel/src/partials/catalog-article.hbs
##########
@@ -0,0 +1,12 @@
+<article class="doc">

Review comment:
       ```suggestion
   <article class="doc catalog">
   ```

##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {
+  margin-top: 20px;
+  display: flex;
+  flex-wrap: wrap;
+}
+
+.catalog-entry {
+  width: 250px;
+  min-height: 140px;
+  padding: 15px;
+  margin: 5px;

Review comment:
       Try using `rem` here, I need to see the markup behind this, that seems like it could be simpler and combine `line-height` in `rem` with `min-width`. Quite possibly some of the padding/margin could come form variables in `vars.css` and be computed.

##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {
+  margin-top: 20px;
+  display: flex;
+  flex-wrap: wrap;
+}
+
+.catalog-entry {

Review comment:
       ```suggestion
   .catalog .entry {
   ```

##########
File path: antora-ui-camel/src/css/img.css
##########
@@ -0,0 +1,8 @@
+article h1.page img {
+  width: 1em;
+}
+
+.nav-link .image > img,
+.breadcrumbs li .image > img {
+  width: 1em;
+}

Review comment:
       Not sure about this change, is it related to catalog? Should go to the `catalog.css` and scoped on `.catalog`.

##########
File path: antora-ui-camel/src/css/catalog.css
##########
@@ -0,0 +1,26 @@
+.catalog-container {
+  margin-top: 20px;
+  display: flex;
+  flex-wrap: wrap;
+}
+
+.catalog-entry {
+  width: 250px;
+  min-height: 140px;
+  padding: 15px;
+  margin: 5px;
+  border-style: solid;
+  border-color: #eee;
+  border-width: 2px;
+  text-align: center;
+}
+
+.catalog-entry .image {
+  display: block;
+  text-align: center;
+}
+
+.catalog-entry .image > img {
+  height: 50px;
+  margin: 15px;

Review comment:
       `line-height` based `rem` size could be used here.




----------------------------------------------------------------
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