Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Docs: update docs design #931

Merged
merged 1 commit into from
May 23, 2023
Merged

Conversation

yasell
Copy link
Contributor

@yasell yasell commented Apr 17, 2023

This PR updates docs UI and styles. Preview

@yasell yasell requested a review from a team as a code owner April 17, 2023 13:30
@yasell yasell requested a review from tpapagian April 17, 2023 13:30
@yasell yasell marked this pull request as draft April 17, 2023 13:31
@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit f07b926
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/646ce8cf645caf00080805ec
😎 Deploy Preview https://deploy-preview-931--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mtardy mtardy requested review from mtardy and removed request for tpapagian April 18, 2023 07:19
@mtardy mtardy added the area/documentation Improvements or additions to documentation label Apr 18, 2023
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Hey thanks for opening that PR, the preview makes it super nice to see. I see we have just a small issue with the commit signing thing currently (see this doc page), you mostly need to use commit --amend -s.

I let you put the PR in "ready for review" mode before making comments about the content :)

@nerucheva
Copy link

nerucheva commented Apr 24, 2023

Here is a preview of content blocks

@mtardy
Copy link
Member

mtardy commented Apr 24, 2023

Hey, I'm just seeing that you modified content under _vendor, you should avoid this since these files are just the theme from upstream, and we might need to update that in the future with theme updates. You can override any things from the theme by putting it directly under the top layouts folder and etc... For CSS, the custom files should be loaded after so that you can replace existing classes. Please tell me if that's not possible for you but otherwise, we will make the website un-updatable.

@nerucheva
Copy link

Hey, I'm just seeing that you modified content under _vendor, you should avoid this since these files are just the theme from upstream, and we might need to update that in the future with theme updates. You can override any things from the theme by putting it directly under the top layouts folder and etc... For CSS, the custom files should be loaded after so that you can replace existing classes. Please tell me if that's not possible for you but otherwise, we will make the website un-updatable.

Got it, I'm fixing it

@yasell yasell marked this pull request as ready for review April 28, 2023 07:57
@yasell yasell requested a review from willfindlay as a code owner April 28, 2023 08:19
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I think it looks great and I guess it will fit well with the new homepage!

However, I have a few topics to discuss:

  • I already addressed that in a separate thread on font and spacing but we need to improve readability I think and this is the main issue, because pages like that do not look very readable I think.
  • I'm not sure having the same(blue) color for quotes and notes is a good idea. You could remove the "info" thing since we can use "note" for that we use the gray color for the quote.
  • Maybe we can do better on the style of <figure>, you have an example in your overview test paragraphs and the figure caption does not look super great.
  • Just to know, have you looked at the theme for code block or not really? I could take a look to see if we can find something better (colors and font). It's pretty ok as-is.
  • This is a nit: I saw you removed a lot of empty lines at the end of files. I'm not sure it's necessary or wanted.
  • Since you changed the "note", "warning", etc. thing to always be labeled on top, we now have a spacing difference between paragraph and title when using a single paragraph block and a multi-paragraph block (look at the diff between caution and note, more visible with Chrome/Firefox tools):
    image
  • on a meta-level: you will need to do some work on your branch:
    • please drop the merge commits with main and just rebase on top of current main. That's why we got the request for review from William because you included a lot of unrelated changes via a merge commit in your branch. You can use git rebase main for that once you have a synched version of main.
    • please squash some of your commits and look at the feedback from the CI that will complain about non-compliant commit messages (too long, mistakes, no signature, etc.). We won't need the commit history from the modification of the vendor folder, then restoration of the vendor folder. I think we could end up with less than 5/10 commits instead of 50. For that you can use git rebase -i FIRST_COMMIT_HASH^. And use squash, reword, etc...

Copy link
Member

Choose a reason for hiding this comment

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

Can we put this file modification on a top level layouts folder in the repository?

Comment on lines 1 to 2
$google_font_name: "Manrope";
$google_font_family: "Manrope:400,600,700,800";
Copy link
Member

Choose a reason for hiding this comment

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

So, while the overall look is great, I'm not sure this font is super readable, it feels like the horizontal spacing is a bit crushed in some parts and everything is really dense (like in the warning), could we do something about it? Or have an alternative?

First example

image

Second example

image

Third example, worse with italic fonts

image

description: >
Discover Tetragon and its capabilities
---

<!-- TODO: remove test content below before merge -->
Copy link
Member

Choose a reason for hiding this comment

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

eventually, we will need to remove this, we can keep this for review.

Comment on lines 1 to 2
$google_font_name: "Manrope";
$google_font_family: "Manrope:400,600,700,800";
Copy link
Member

Choose a reason for hiding this comment

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

Idk if in general, we could benefit from a bit more spacing, page can look very dense, although if you are applying best practices in typography feel free to ignore my ignorant comment :D

image

@mtardy mtardy removed the request for review from willfindlay April 28, 2023 09:14
@mtardy
Copy link
Member

mtardy commented Apr 28, 2023

Also if we are using icons for next section as you added, is it possible to add a default one for pages that don't define an icon in the frontmatter to avoid this:

image

@nerucheva
Copy link

nerucheva commented May 4, 2023

@mtardy could you please specify what you would like to change in <figure> appearance?
I see that it looks like the margin is too big, but the reason is that the image has a white background. Cropping the image will solve this problem.
Changes for the color and size of <figcaption> will be pushed soon. Let me know please if there is something else to be changed.

Снимок экрана 2023-05-04 в 14 20 09

Снимок экрана 2023-05-04 в 14 20 31

@mtardy
Copy link
Member

mtardy commented May 4, 2023

@mtardy could you please specify what you would like to change in <figure> appearance? I see that it looks like the margin is too big, but the reason is that the image has a white background. Cropping the image will solve this problem. Changes for the color and size of

will be pushed soon. Let me know please if there is something else to be changed.
Снимок экрана 2023-05-04 в 14 20 09

Снимок экрана 2023-05-04 в 14 20 31

Ah! I had no idea specific in mind but just saw that it was not really on the same level of design as the rest. If you think it's pairing well with the rest let's use the default.

@mtardy
Copy link
Member

mtardy commented May 4, 2023

Also, can we add a favicon? :) Teteragon logo should be fine in there.

@nerucheva nerucheva force-pushed the docs-design branch 3 times, most recently from 6650f06 to e42ee97 Compare May 5, 2023 08:46
@nerucheva
Copy link

@mtardy hi! Please check the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Hey, you again modified files under _vendor, same for the favicons. Place them in the top-level layouts and static folders, please.

@@ -1,6 +1,7 @@
---
title: "Kubernetes quickstart guide"
weight: 2
weight: 1
Copy link
Member

Choose a reason for hiding this comment

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

please do not change the organization of the doc by changing weight!

Comment on lines 15 to 16
max-width: 800px;
margin: 0 auto;
Copy link
Member

Choose a reason for hiding this comment

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

I saw that you added that, I think it's quite unusual for docs website to be centered so maybe we should remove the margin: 0 auto. And is the max-width based on typography rules? I think it's a really nice addition since websites like k8s.io/docs don't have it and might look weird on large screens. We could also use a unit like ch to adjust the max-width to an average maximum number of words per line?

>}}

The documentation is divided into the following sections:
{{< banner title="Getting Started with Tetragon" content="Quickly get started and learn how to install, deploy and configure it" link="getting-started/" >}}
Copy link
Member

Choose a reason for hiding this comment

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

the link seems to not work on getting started button in the preview.

@mtardy
Copy link
Member

mtardy commented May 5, 2023

Let's take my comment and see how it evolved :)

Thanks a lot! I think it looks great and I guess it will fit well with the new homepage!

However, I have a few topics to discuss:

I think on that topic, the new font is way easier to read! It's less customized and simpler but honestly, I find it better thanks!

I think we could still use a little bit more spacing around headers for examples, and other particular objects (paragraphs, lists) since it looks a bit dense still. I don't know if it's only me but the default theme was too much spacing around h1 and h2 headers but this still looks pretty dense. Again if you are following typography principles feel free to educate me since it's just my personal very objective opinion and it can be very wrong.

image

  • I'm not sure having the same(blue) color for quotes and notes is a good idea. You could remove the "info" thing since we can use "note" for that we use the gray color for the quote.

Ok, thanks! Looks okay

  • Maybe we can do better on the style of <figure>, you have an example in your overview test paragraphs and the figure caption does not look super great.

Looks okay

  • Just to know, have you looked at the theme for code block or not really? I could take a look to see if we can find something better (colors and font). It's pretty ok as-is.

I saw you personalized the monospace font with $font-family-monospace: "IBM Plex Mono", monospace !default;. I guess it's to go well with the other font?

you did not change that but it's ok.

  • Since you changed the "note", "warning", etc. thing to always be labeled on top, we now have a spacing difference between paragraph and title when using a single paragraph block and a multi-paragraph block (look at the diff between caution and note, more visible with Chrome/Firefox tools):
    image

Thanks, it looks great now.

  • on a meta-level: you will need to do some work on your branch:

    • please drop the merge commits with main and just rebase on top of current main. That's why we got the request for review from William because you included a lot of unrelated changes via a merge commit in your branch. You can use git rebase main for that once you have a synched version of main.
    • please squash some of your commits and look at the feedback from the CI that will complain about non-compliant commit messages (too long, mistakes, no signature, etc.). We won't need the commit history from the modification of the vendor folder, then restoration of the vendor folder. I think we could end up with less than 5/10 commits instead of 50. For that you can use git rebase -i FIRST_COMMIT_HASH^. And use squash, reword, etc...

It looks like you squashed everything under "squash commit" 😅 usually the idea is to have commits of things that make sense together and have a nice description for the future addition, maybe we can take over on that and squashed the way we think it makes sense if it's not practical for you. In the end, it's ok if we end up with only one commit and all the history, and interesting stuff in the commit message. Really it's up to you and how you think the changed should be organized.

@mtardy
Copy link
Member

mtardy commented May 5, 2023

btw you haven't touched the background/borders of code block, what's your opinion on that, do you think the default look great? I personally appreciate that text code blocks are different than code blocks (one without and one with borders)

image

I think the one with the border does not look very modern but if we modify them it would be nice if we can still differentiate them easily (since text is usually output and code is input). And also if we can add a "copy" button on the right of the code block for people to easily copy-paste examples it would be amazing (not on text code block, but only on code code block :D). Do you think it's doable?

@mtardy
Copy link
Member

mtardy commented May 11, 2023

btw what happened to the search button on the top left? :)

@mtardy
Copy link
Member

mtardy commented May 11, 2023

I was thinking we could add https://github.com/google/docsy/blob/main/userguide/content/en/docs/adding-content/shortcodes/index.md to see a lot of the shortcode used and their look. I think one we skipped is the card tab:

image

@nerucheva nerucheva force-pushed the docs-design branch 2 times, most recently from c240da9 to b07608f Compare May 16, 2023 14:03
@mtardy
Copy link
Member

mtardy commented May 22, 2023

Hey @nerucheva! We would love to merge this as it's in a good state. If we want to make improvements we can iterate from there. Can you do two things so that it's ready to merge:

  • Squash this PR into one signed-off commit with a recap like (this is a suggestion, write what fits the most) "docs: update style and design", you can let an empty line and then write whatever you think is interesting.
  • Remove the example page.

Signed-off-by: Raman Yasel <raman.yasel@gmail.com>
@yasell
Copy link
Contributor Author

yasell commented May 23, 2023

Hi @mtardy!

I've done everything you asked Tatiana to do above☝️, please review

@michi-covalent
Copy link
Contributor

is the button green? yes.

@michi-covalent michi-covalent merged commit 220974d into cilium:main May 23, 2023
@yasell yasell deleted the docs-design branch May 23, 2023 20:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants