Update website styling and add Google site verification #408

Badbond · 2022-12-12T12:39:51Z

Extracted from the website branch. Therefore, this is effectively already applied apart from a single selective tweak to which I added a comment.

As review, please make sure I included everything in scope. Considered out of scope:

Documentation generation (docgen) from source code using a compiler plugin.
Ruby setup built on top of docgen to template the Markdown pages.
GitHub workflow changes.

Suggested commit message (feel free to tweak):

Update website styling and assets (#408)

Also credits to @oxkitsune and @japborst as authors for some of these changes.

Badbond · 2022-12-12T12:41:44Z

website/_sass/color_schemes/_variables.scss

+// Overrides for Just the Docs, see
+// https://github.com/just-the-docs/just-the-docs/blob/main/_sass/support/_variables.scss.


I slightly rephrased this compared to the website branch.

github-actions · 2022-12-12T12:41:50Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

W.r.t. the favicon, I see we added a new one, should we also delete the other?

Left some comments and suggestions :).

rickie · 2022-12-12T13:16:45Z

website/_includes/head_custom.html

 <link rel="stylesheet" href="{{ '/assets/css/just-the-docs-eps-light.css' | relative_url }}"
-    media="(prefers-color-scheme: light)">
+      media="(prefers-color-scheme: light)">


These spaces seem a bit weird 👀. Is that because of a formatter?

Seems to have changed during cherry-pick from @oxkitsune's branch to mine. This formatting happens to me as part of IntelliJ formatting (CTRL-ALT-L). Currently no CI formatting/linting is in place for these files, nor could I find any so quickly to run it as a one-off. I'm happy with reverting if you'd like. If we go with IntelliJ formatting for now, I'll remove the space at the last line before /> too (committed).

Yeah let's do IntelliJ formatting for now :).

website/_sass/color_schemes/eps-dark.scss

website/_sass/color_schemes/_variables.scss

website/_includes/head_custom.html

website/README.md

website/_includes/head_custom.html

rickie · 2022-12-12T13:33:00Z

website/_sass/color_schemes/_common.scss

@@ -1,5 +1,25 @@
-footer > img#logo {
+// Add support for external anchor icons.


This "style" of writing sounds like an XXX to me. WDYT about changing it to: "Adds support for...". Not sure if that is the right way to go, but maybe it makes more sense 🤔.

I think such comments are similar to in-code comments (Java example) which can carry the same mood. It is just a hard-to-explain feeling, but to me the style of 'Adds' is a bit awkward in this context and feels more fitting in JavaDoc-type of comments. 😅 But I'd be happy to change this, but then it should be applied in other places too.

We could meet in the middle and use /*-style comments here? No strong opinion though. (I agree that the grammar is correct as-is!)

There is a difference in behavior between //-style and /* style commments, though. // do not get compiled to CSS and /* do end up in the compiled CSS. See SASS documentation. Not sure whether it is worth it while sending more bytes over the line.

TIL! Agree that we can then leave it as-is.

Badbond · 2022-12-12T14:03:11Z

To answer many of your questions @rickie. Many changes originate from @oxkitsune and @japborst in the gdejong/docgen_old branch squashed/cherry-picked in the pdsoels/docgen_final branch (which consolidates even more branches). This docgen-final branch was used as a baseline for the as-we-now-know-it website branch (from which these changes are selectively fetched/filtered).

Edit: However, likely master has changed out-of-sync with those docgen branches in #253. So some changes might effectively revert changes merged directly into master. It will be some effort to time-track for every changed line which came first, so let's go with what we like most.

github-actions · 2022-12-12T14:23:41Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-12T14:28:45Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-12T14:31:32Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Badbond · 2022-12-12T15:49:21Z

website/_sass/color_schemes/_common.scss

+  &:first-of-type {
+    margin-left: 0;
+  }


@japborst is this change still necessary? It renders fine without for me.

This reduces the margins when you have multiple labels. What do you mean with "renders fine"?

I should be more specific. My human eye can't see the difference.

With:

Without:

It seems that the lines above with !important override the behavior. Perhaps we should change this entire section to:

.label { margin-left: 0 !important; }

But maybe there is a more proper way to do this such that we don't need !important, but I am no CSS expert either 😅

As discussed offline, added a commit for this suggestion and updated the comment next to it.

github-actions · 2022-12-12T16:17:17Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

japborst · 2022-12-12T19:59:31Z

website/_includes/nav_footer_custom.html

@@ -0,0 +1 @@
+<!-- This file removes the just-the-docs reference in the nav bar's footer. -->


Why do we want to remove it? It's OSS software and don't see why we would remove the reference to it.

It's what we did on the website branch, I believe because we thought it to be cleaner at the time. I am fine with removing it to give some credits, if others agree.

Can't recall adding this, but might have just forgotten. Let's add it back.

Added back.

japborst · 2022-12-12T20:00:41Z

website/_sass/color_schemes/_common.scss

+  &:first-of-type {
+    margin-left: 0;
+  }


This reduces the margins when you have multiple labels. What do you mean with "renders fine"?

Stephan202 · 2022-12-13T06:08:54Z

The build fails for reasons unrelated to this PR; we should merge #410 first. (I'll review this PR once a few approvals are in, since I'm not an expert on this stuff.)

Badbond · 2022-12-13T11:52:19Z

Rebased.

github-actions · 2022-12-13T11:54:16Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Badbond · 2022-12-13T13:15:32Z

@rickie it seems like we can't delete website/favicon.ico (see build failure). There are still references to it in the generated content. When looking at the sources of https://error-prone.picnic.tech/, I can also see a reference to /favicon.ico as well as /assets/images/favicon.ico in the head. As documented here, the contents of custom_head are appended. So we should either keep it in /website/favicon.ico or ignore this specific link in HTML proofer. WDYT?

github-actions · 2022-12-13T13:54:11Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-13T13:56:39Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2022-12-15T09:03:54Z

Based on the documentation link, shouldn't we also add type="image/x-icon"> to the favicon line?

So IIUC if we would merge this while it's red, it should turn green after it's on master and an actual link right? So now it will complain because it doesn't exist yet?

github-actions · 2022-12-15T10:51:37Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Badbond

Based on the documentation link, shouldn't we also add type="image/x-icon"> to the favicon line?

I can't quickly find a clear answer what it is used for, but can't hurt to add. Added it.

So IIUC if we would merge this while it's red, it should turn green after it's on master and an actual link right? So now it will complain because it doesn't exist yet?

As discussed offline, no it probably will not turn green. The reference to /favicon.ico in the head is added by either Jekyll or just-the-docs, our reference to /assets/images/favicon.ico in head_custom is appended to the the head, causing the favicon link to be overwritten. I found no way of removing the former link. As discussed, I now excluded /favicon.ico from validation.

Badbond · 2022-12-15T10:54:25Z

.github/workflows/deploy-website.yaml

@@ -28,7 +28,7 @@ jobs:
        working-directory: ./website
        # XXX: Drop `--disable_external true` once we fully adopted the
        # "Refaster rules" terminology on our website and in the code.
-        run: bundle exec htmlproofer --disable_external true --check-external-hash false ./_site


Execution of htmlproofer will eventually live in a Ruby script instead.

The favicon.ico should be in the root for best browser support, sorry 😕 The error was actually correct by htmlproofer 😉

See here for more details https://realfavicongenerator.net/faq#why_icons_in_root

rickie

Changes LGTM!

japborst

👌

Badbond · 2022-12-15T12:56:59Z

As discussed offline, we had the root favicon.ico for supporting browsers which ignore the specific favicon path. I reverted the changes made before and instead drop the one in the assets/images directory, including the head_custom line referring to it.

Badbond · 2022-12-15T12:57:21Z

Ready for your eyes @Stephan202.

github-actions · 2022-12-15T12:57:38Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Badbond · 2022-12-20T12:46:13Z

Rebased.

github-actions · 2022-12-20T12:47:44Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2022-12-27T06:07:29Z

Rebased.

github-actions · 2022-12-27T06:09:05Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2022-12-27T06:23:37Z

(Didn't forget about this PR; will certainly find a moment in the coming days.)

Co-authored-by: Rick Ossendrijver <[email protected]>

As opposed to having a custom link to `/assets/images/favicon.ico`.

Stephan202

Rebased and added a small commit. Left a few questions, and didn't scrutinize the CSS too much, but LGTM :)

Stephan202 · 2022-12-29T07:08:30Z

website/_config.yml

@@ -36,6 +42,7 @@ social:
    - https://github.com/PicnicSupermarket
    - https://twitter.com/picnic
    - https://www.linkedin.com/company/picnictechnologies
+


I'd omit this empty line, because the twitter section below is also part of the SEO config mentioned above.

Stephan202 · 2022-12-29T07:08:49Z

website/_config.yml

+callouts:
+  summary:
+    color: blue
+  note:
+    color: grey-dk


Just checking: this will only be used in the next PR, right?

Correct. All changes in this PR won't be effective on master yet but are (almost all -- cases where we didn't, I tested locally against the website branch) already present on the website branch and therefore deployed. After @rickie's docgen modules, I'll open a PR to move the Ruby templating logic and a subsequent PR to change the GHA logic.

Stephan202 · 2022-12-29T07:14:59Z

website/_sass/color_schemes/_common.scss

@@ -1,5 +1,25 @@
-footer > img#logo {
+// Add support for external anchor icons.


We could meet in the middle and use /*-style comments here? No strong opinion though. (I agree that the grammar is correct as-is!)

Stephan202 · 2022-12-29T07:19:46Z

website/_sass/color_schemes/eps-dark.scss

+$grey-dk-000: $grey-lt-000;
+$grey-dk-100: $grey-lt-100;
+$grey-dk-200: $grey-lt-200;
+$grey-dk-250: $grey-lt-200;
+$grey-dk-300: $grey-lt-300;


Just checking: only 250 having an "off' mapping is intended?

I think it is based on the colors defined by just-the-docs. I believe the idea was for the summary boxes to be light instead of dark in a dark theme, as otherwise it does not stand out as much. As you can see, the light grays only contain 4 entries instead of 5.

Wow 😄 (Maybe worth a comment; not sure.)

Makes sense. Added a comment 👍.

github-actions · 2022-12-29T07:22:10Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-29T08:19:39Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-29T18:03:17Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added one more commit. Updated suggested commit message (IIUC there aren't really asset changes here):

Update website styling and add Google site verification (#408)

Stephan202 · 2022-12-29T17:56:29Z

website/_sass/color_schemes/_common.scss

@@ -1,5 +1,25 @@
-footer > img#logo {
+// Add support for external anchor icons.


TIL! Agree that we can then leave it as-is.

Badbond commented Dec 12, 2022

View reviewed changes

rickie reviewed Dec 12, 2022

View reviewed changes

rickie requested a review from japborst December 12, 2022 13:34

Badbond commented Dec 12, 2022

View reviewed changes

japborst reviewed Dec 12, 2022

View reviewed changes

Badbond force-pushed the pdsoels/PSM-1720 branch from 783fb77 to 1e98ce5 Compare December 13, 2022 11:52

rickie added the documentation A documentation update label Dec 13, 2022

rickie added this to the 0.7.0 milestone Dec 13, 2022

Badbond commented Dec 15, 2022

View reviewed changes

rickie approved these changes Dec 15, 2022

View reviewed changes

Badbond requested a review from japborst December 15, 2022 12:37

japborst approved these changes Dec 15, 2022

View reviewed changes

Badbond requested a review from Stephan202 December 15, 2022 12:57

Badbond force-pushed the pdsoels/PSM-1720 branch from cd13d3e to 7f36364 Compare December 20, 2022 12:45

rickie force-pushed the pdsoels/PSM-1720 branch from 7f36364 to 873545c Compare December 27, 2022 06:07

Badbond and others added 11 commits December 29, 2022 08:20

Update website styling

1abbdee

Suggestion

0a59d73

Co-authored-by: Rick Ossendrijver <[email protected]>

Apply suggestion

1f65577

Co-authored-by: Rick Ossendrijver <[email protected]>

Apply comment suggestions and delete unused favicon.ico

23396d8

Improve comment formatting

5a8b26d

Apply README.md suggestion

981e552

Drop nav_footer_custom and format

10cd374

Add link type and ignore /favicon.ico URLs

94e54de

Use root favicon.ico

42ced08

As opposed to having a custom link to `/assets/images/favicon.ico`.

Align labels with surrounding elements

3aadc24

Suggestions

b670c93

Stephan202 force-pushed the pdsoels/PSM-1720 branch from 873545c to b670c93 Compare December 29, 2022 07:20

Stephan202 reviewed Dec 29, 2022

View reviewed changes

Badbond requested a review from Stephan202 December 29, 2022 08:14

Add comment for overriding grey values in dark theme

29d700f

Tweak

50ec5ac

Stephan202 approved these changes Dec 29, 2022

View reviewed changes

rickie changed the title ~~Update website styling and assets~~ Update website styling and add Google site verification Dec 30, 2022

rickie merged commit 601fcf2 into master Dec 30, 2022

rickie deleted the pdsoels/PSM-1720 branch December 30, 2022 07:46

		// Overrides for Just the Docs, see
		// https://github.com/just-the-docs/just-the-docs/blob/main/_sass/support/_variables.scss.

		@@ -1,5 +1,25 @@
		footer > img#logo {
		// Add support for external anchor icons.

		@@ -0,0 +1 @@
		<!-- This file removes the just-the-docs reference in the nav bar's footer. -->

Update website styling and add Google site verification #408

Update website styling and add Google site verification #408

Conversation

Badbond commented Dec 12, 2022 • edited by rickie Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2022

rickie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond commented Dec 12, 2022 • edited Loading

github-actions bot commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Dec 13, 2022

Badbond commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

Badbond commented Dec 13, 2022 • edited Loading

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

rickie commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

Badbond left a comment • edited by rickie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

japborst left a comment

Choose a reason for hiding this comment

Badbond commented Dec 15, 2022

Badbond commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

Badbond commented Dec 20, 2022

github-actions bot commented Dec 20, 2022

rickie commented Dec 27, 2022

github-actions bot commented Dec 27, 2022

Stephan202 commented Dec 27, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2022

github-actions bot commented Dec 29, 2022

github-actions bot commented Dec 29, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond commented Dec 12, 2022 •

edited by rickie

Loading

rickie left a comment •

edited

Loading

Badbond Dec 12, 2022 •

edited

Loading

Badbond commented Dec 12, 2022 •

edited

Loading

Badbond commented Dec 13, 2022 •

edited

Loading

Badbond left a comment •

edited by rickie

Loading

Badbond Dec 29, 2022 •

edited

Loading

Badbond Dec 29, 2022 •

edited

Loading