Adding a navigational structure based on aip.dev by garrettjonesgoogle · Pull Request #1253 · GoogleCloudPlatform/cloud-opensource-java · GitHub

This adds site search functionality
I tested it on both desktop and "virtual mobile" (the Chrome devtools mobile view)

fixes #885
fixes #992
fixes #1034

@suztomo @elharo at least 1 reviewer should pull this locally and run it in Jekyll, then poke around to make sure everything is in order

Checking out your branch...

From https://github.com/GoogleCloudPlatform/cloud-opensource-java/wiki/Tools#jekyll-for-github-pages:

# In docs directory
bundle install --path vendor/bundle
bundle exec jekyll serve

It worked.

I didn't find any missing link.

I find this "Browse best practices" link redundant, because the logo "JLBPs" goes to the same top page.

It is technically redundant, but the explicit link helps people find what they are looking for easier - clicking on the logo might not be obvious. Also, I would argue that we shouldn't couple together the root of the site with the JLBP index - we might want to separate those ideas apart later. Lastly, if we add more on the header later, it will feel more "even" (look at the header of aip.dev).

Yes, that button may be useful in future, but not now. We can easily add it back to the site when needed.

@elharo what do you think?

I was thinking of an idea for headers to add. What about:

Best practices (the existing one)
Articles
Reference
Tools (new page - discuss the static linkage checker)

Also:

Talks (e.g. https://saturnism.me/talk/surviving-dependency-hell-maven/)

Good idea. Let's put that in a separate PR though.

Doing a separate PR seems fine for the header links I suggested here - but how about the single header link that is already present ("Browse best practices") which @suztomo wants to remove?

The header bar and site search is the part of this PR I'm not sold on. The navigation and sidebar is a much more obvious win. I think we can move that forward quickly without having to vet all the JavaScript that's being added for Tipue.

I can remove the search functionality if that's a sticking point. However, there is one key problem with removing the whole header - it's what provides the link to navigate to the root of the site, and on mobile, it's the only way to navigate away from the current page (through the hamburger menu).

The results look good. Internally this is a major increase in CSS complexity. I'm not sure I understand it all and I'm going to need some time to look through it, so I'm going to add a temporary hold.

Why do you need to understand it? It's CSS... which I never understand...

I'm unsure about the top bar. Can we move that to a separate PR and make this PR just the sidebar and navigation?

The top bar has the new search functionality, which seems like a very useful feature.

At a high level, I tried to minimize the work to create this navigational structure by starting with aip.dev's structure and stripping out stuff we don't need. That process was pretty straightforward. Gutting the top bar would require more work, then we'd need even more work to put back a top bar later when we want one.

Basically, I view the current structure in the PR as an MVP, and further changes are improvements on top of it.

my comnets are:

remove the right-hand nav, seems redundant
left-hand nav has Best Practices -> Best Practices Index, seems redundant as well.
articles - I feel the current articles are actually intro info, as oppose to blogs.

I actually do like the site as-is, and agreed that it could use a backlink.

Proposed left-nav:

Introduction
Cause of Dependency Conflicts
Compiler and Dependency Conflicts
Best Practices
[JLBP-1] ...
...
References
Glossary

@saturnism :

For longer articles, the table of contents can be helpful to navigate to specific sections. This is standard on cloud.google.com - e.g. https://cloud.google.com/billing/docs/concepts
I'm fine removing the "best practice index" link.
I agree that the articles are not blogs (which is why I didn't use the term "blogs"). Do you think the term "articles" is misleading? I plan on writing additional articles, which will be less "introductory" the more I write.
I personally like the idea of the intro articles at the top, but @elharo objected to putting them above the best practices in a prior PR of mine. Also, as I continue to write more articles, the list will increase in size. We may not want to hide the best practice list as the article list grows.

@saturnism :

For longer articles, the table of contents can be helpful to navigate to specific sections. This is standard on cloud.google.com - e.g. https://cloud.google.com/billing/docs/concepts

The standard on c.g.c (namely concepts) doesn't mean it's a best practice...

I'm fine removing the "best practice index" link.

+1 . The index may just be the home page itself as is today.

I agree that the articles are not blogs (which is why I didn't use the term "blogs"). Do you think the term "articles" is misleading? I plan on writing additional articles, which will be less "introductory" the more I write.

I do feel articles is misleading; otherwise, articles become a long list of dumping ground for any new content; grouping them logically structure is better, and we can be more focused on what content to add.

I personally like the idea of the intro articles at the top, but @elharo objected to putting them above the best practices in a prior PR of mine. Also, as I continue to write more articles, the list will increase in size. We may not want to hide the best practice list as the article list grows.

I can understand not having Introudction over-shadow the rules themselves. I also dislike a lot of c.g.c article put "intro" or "concepts" at the end ;)

How do we group these articles together? I see them as introduction, or just info on Dependency Conflicts.

Best Practices
More on Dependency Conflicts
- What causes it
- Why compiler cannot detect it
- ... add more as we go.

@saturnism :

For longer articles, the table of contents can be helpful to navigate to specific sections. This is standard on cloud.google.com - e.g. https://cloud.google.com/billing/docs/concepts

The standard on c.g.c (namely concepts) doesn't mean it's a best practice...

I'm not sure why best practices matter exactly. Consistency itself is a best practice anyway, right? What best practices are you trying to enforce here?

I'm not sure why you are referencing "concepts" in particular, besides the fact that it's the link I gave; you can visit any page under cloud.google.com and see the TOC on the right, e.g. https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types .

@saturnism :

I personally like the idea of the intro articles at the top, but @elharo objected to putting them above the best practices in a prior PR of mine. Also, as I continue to write more articles, the list will increase in size. We may not want to hide the best practice list as the article list grows.

I can understand not having Introudction over-shadow the rules themselves. I also dislike a lot of c.g.c article put "intro" or "concepts" at the end ;)

How do we group these articles together? I see them as introduction, or just info on Dependency Conflicts.

Best Practices

More on Dependency Conflicts

What causes it

Why compiler cannot detect it

... add more as we go.

What do you think about me using "Concepts" instead of "Articles"? (I'm looking for a short name so that I can stick it on the header later)

Do you feel that the article titles should be in the sidebar, or is a link to a Concepts index page sufficient?

…-opensource-java

I have made some updates:

Removed the search functionality
Removed the "Browse best practices" link from the header
Removed the "Best practices index" from the sidebar
Renamed "Articles" to "Concepts"

PTAL

What does this script do?

@lukesneeringer could you help us understand what the scripts in https://github.com/googleapis/aip/blob/master/_includes/aip-closing-scripts.html do and why we might need them?

Offline answer:

The inline one makes the header bar work on mobile.
The other two are what Glue tells you to use. https://glue-docs.appspot.com/get-started/cdn/

I will add comments to the file explaining this.

stet. This is en-US, not site.lang

I don't understand what you're asking for...

I will revert this line.

which of these do we actually use? I don't see .tldr referenced anywhere for example

We don't use them yet, but I thought they could be useful to incorporate.

I will remove the callout stuff since it's not used.

why not? It it's not editable it shouldn't be in the repo.

I'm not sure what your objection is... it's pretty standard practice to put warnings on files that are derived from other sources. Technically such files can be edited, but it's not recommended because it's much harder to maintain.

I pulled out the page metadata and dynamic index into #1270 .

The meta description is incorrect:

<meta name="description" content="Code health dashboard for GCP open source projects" />
<meta property="og:description" content="Code health dashboard for GCP open source projects" />

In the exiting site it is

<meta name="description" content="Tools for detecting and avoiding linkage errors in GCP open source projects" />
<meta property="og:description" content="Tools for detecting and avoiding linkage errors in GCP open source projects" />

which is a little better, but we could improve this.

When I run this for myself, I see the correct one. This seems to be coming from the GitHub repo and not from anything under the docs/ directory.

Addressed feedback (except one item where I have a question), PTAL

I need to run the site again, but it looks like there's more navigation inside main, e..g jump-content? Maybe that's the rightand sidebar? Main is for the content only. This is important for accessibility and rendering on non-desktop screens.

Done.

PTAL, all navigation stuff has been moved outside of <main>.

The gray text in some headings on the page fails accessibility checks. That is, it's hard to read for people with less than perfect vision. It needs higher contrast, preferably black text:

Done - Footer updated to black.

…-opensource-java

@elharo what browser are you using to check accessibility?

That was firefox. Couldn't get Chrome to work.

…

On Fri, Mar 6, 2020 at 11:09 AM Garrett Jones ***@***.***> wrote: @elharo <https://github.com/elharo> what browser are you using to check accessibility? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1253?email_source=notifications&email_token=AAHVP2HCH47EPN7XARR3IILRGEN27A5CNFSM4K4MQ3YKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOB4RFQ#issuecomment-595839126>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHVP2F2FSPXAUDP5LTUAVTRGEN27ANCNFSM4K4MQ3YA> .

-- Elliotte Rusty Harold elharo@macfaq.com

Titles in left sidebar are still gray on gray. E.g "Best Practices"

There are two title elements in the head.

<html lang="en-US" class="glue-flexbox">
--
  | <head>
  | <title>Google Best Practices for Java Libraries</title>
  | <meta charset='UTF-8'>
  | <meta http-equiv="X-UA-Compatible" content="IE=edge">
  | <meta name="viewport" content="width=device-width,maximum-scale=2">
  | <link href="https://fonts.googleapis.com/css?family=Roboto:100,300,400,500,700\|Google+Sans:400,500\|Product+Sans:400&amp;lang=en" rel="stylesheet"></link>
  | <link rel="stylesheet" type="text/css" media="screen" href="https://www.gstatic.com/glue/v22_1/glue.min.css">
  | <link rel="stylesheet" type="text/css" media="screen" href="/assets/css/style.css?v=20deac654f99e81a3e313819fe82ba3d87edbb4d">
  | <link rel="stylesheet" type="text/css" media="print" href="/assets/css/print.css?v=20deac654f99e81a3e313819fe82ba3d87edbb4d">
  | <script type="text/javascript" src="https://code.jquery.com/jquery-3.4.0.min.js"></script>
  | <script type="text/javascript" src="https://www.gstatic.com/glue/latest/glue-detect.min.js"></script>
  | <script type="text/javascript">
  | $('html').css('visibility', 'hidden');
  | $.when($.ready).then(() => {
  | $('html').css('visibility', 'visible');
  | });
  | </script>
  | <script type="text/javascript" src="/assets/js/global.js?v=20deac654f99e81a3e313819fe82ba3d87edbb4d"></script>
  | <!-- Begin Jekyll SEO tag v2.5.0 -->
  | <title>Google Best Practices for Java Libraries \| Code health dashboard for GCP open source projects</title>

The gray in the left sidebar titles is now fixed
The redundant title tag in the head tag is now removed

Contrast is still below 4.5 for the titles on the lefthand side. The weird background highlighting might have something to do with that.

There's also a warning about the header and keyboard navigation I don't understand yet.

I fixed the contrast for highlighted sidebar links.

I'm not sure about the keyboard warnings either. I believe they come from things put there by Glue. It all follows the template provided by Glue at https://glue-docs.appspot.com/docs/components/raw/header#HTML .

Why do we have both a big View on Github Button and a View Page on Github link?

The link alone is probably sufficient. The primary use case here are readers, not writers and I'd prefer not to put more chart junk on the page.

And once that's done, do we really need the top bar at all? Maybe just move the logo into the top of the lefthand nav bar, and then more of the JLBPs fit above the fold.

learn --> Learn

Ok

Why do we have both a big View on Github Button and a View Page on Github link?
...
The link alone is probably sufficient. The primary use case here are readers, not writers and I'd prefer not to put more chart junk on the page.

And once that's done, do we really need the top bar at all? Maybe just move the logo into the top of the lefthand nav bar, and then more of the JLBPs fit above the fold.

I removed the "View on Github" button, and I found some more dead css to remove.

We still need the top bar:

It is the conventional layout across Google sites to include a header
I intend on adding more links in the header, including a search function, I just didn't include them in this PR to simplify it
The header provides a "jump to content" accessibility link (part of Glue recommendations)
Without the header, mobile has no way to get to the home index. I tried removing the header on desktop only, but Glue seems to depend on header content being available for both desktop/mobile for it to work on mobile at all.
Stripping the header from this PR but then adding it in a following PR creates a bit more unnecessary work

…-opensource-java

Firefox reports four remaining accessibility issues.

This doesn't seem to show up and it issues an accessibility warning:
"Clickable elements must be focusable and should have interactive semantics. Learn more"

Can we delete it?

This is taken straight from the Glue sample:

https://glue-docs.appspot.com/docs/components/raw/header#HTML

Also it only appears on mobile mode. Shrink the window down to see it appear.

"Warning

Focusable elements should have interactive semantics."

This is taken straight from the Glue sample:

https://glue-docs.appspot.com/docs/components/raw/header#HTML

"Error

Interactive elements must be labeled. Learn more"

This is taken straight from the Glue sample: https://glue-docs.appspot.com/docs/components/raw/header#HTML

I guess I don't trust Glue as much as you do. When Firefox scans aip.dev it finds a number of problems I know are real issues, so I'm inclined to give it the benefit of the doubt on the ones I don't immediately understand. If you like, we can dig into these further, though in general I'm more comfortable building up from small pieces of code I understand than trying to cut out the large pieces I don't.

If there are problems with Glue, I think we should get them fixed at the source instead of 1) trying hack fixes on top of Glue or 2) forging our own path. I would argue that fixing problems at the source should not be in the scope of this PR and shouldn't block it, given the value-add nature of the PR. Are you going to set a bar of perfection in order to approve the PR? We are not a front-end team; we are a team trying to help people solve diamond dependency conflicts.

I would guess that the accessibility issues on aip.dev that have been fixed on jlbp.dev are likely local choices and not issues with Glue.

All of these issues are from the sample html that Glue provides. Glue is designed as an accessible framework. I don't want to second-guess Glue given my lack of experience in this area. I'm guessing Firefox isn't perfect here. For example, two of the accessibility items are about the hamburger menu, which is supposed to be absent in the desktop view.

This is taken straight from the Glue sample: https://glue-docs.appspot.com/docs/components/raw/header#HTML

This is taken straight from the Glue sample:

https://glue-docs.appspot.com/docs/components/raw/header#HTML

Also it only appears on mobile mode. Shrink the window down to see it appear.

This is taken straight from the Glue sample:

https://glue-docs.appspot.com/docs/components/raw/header#HTML

Adding a navigational structure based on aip.dev

980288e

googlebot added the cla: yes label Feb 26, 2020

garrettjonesgoogle requested review from elharo and suztomo February 26, 2020 19:40

suztomo reviewed Feb 26, 2020

View reviewed changes

elharo suggested changes Feb 27, 2020

View reviewed changes

elharo reviewed Feb 28, 2020

View reviewed changes

garrettjonesgoogle added 4 commits February 28, 2020 15:56

Removing search functionality

819386d

Renaming 'articles' to 'concepts'

654925e

Removing 'Browse best practices' link in header

d58e69e

Merge branch 'master' of https://github.com/GoogleCloudPlatform/cloud…

146cf94

…-opensource-java

suztomo previously approved these changes Feb 29, 2020

View reviewed changes

This was referenced Feb 29, 2020

Glue #1034

Open

Previous | Home | Next links at bottom of JLBP pages #992

Open

elharo reviewed Mar 2, 2020

View reviewed changes

garrettjonesgoogle mentioned this pull request Mar 3, 2020

Adding page metadata, making index page dynamic #1270

Merged

Removing unused stuff

c88386a

PR feedback

77b5169

PR feedback part 2

f49a7ad

elharo reviewed Mar 5, 2020

View reviewed changes

garrettjonesgoogle added 2 commits March 5, 2020 09:03

Moving all navigational stuff outside of main

2de03ff

Moving role attribute where it goes

81e3e00

elharo reviewed Mar 6, 2020

View reviewed changes

Making footer black instead of grey

157f245

Merge branch 'master' of https://github.com/GoogleCloudPlatform/cloud…

20deac6

…-opensource-java

elharo reviewed Mar 6, 2020

View reviewed changes

garrettjonesgoogle added 2 commits March 6, 2020 10:58

Accessibility improvement

5f61c3f

Removing redundant title tag

cbd9249

elharo reviewed Mar 6, 2020

View reviewed changes

More accessibility improvements

935f857

elharo reviewed Mar 9, 2020

View reviewed changes

PR feedback, removing unnecessary stuff

0ae2c70

Merge branch 'master' of https://github.com/GoogleCloudPlatform/cloud…

4c2519d

…-opensource-java

elharo reviewed Mar 9, 2020

View reviewed changes

garrettjonesgoogle commented Mar 9, 2020

View reviewed changes

garrettjonesgoogle dismissed suztomo’s stale review via 4c2519d June 3, 2025 08:11

+                      <nav class="glue-header__link-bar">
+                        <ul id="list-1" class="glue-header__list">
+                          <li class="glue-header__item{% if page.jlbp or page.jlbp_index %} glue-header--is-active{% endif %}">
+                            <a class="glue-header__link" href="/" aria-level="1">Browse best practices</a>

-                  </script>
-                  {% endif %}
-                </head>
+              <html lang="{{ site.lang | default: 'en-US' }}" class="glue-flexbox">

+                    {% include jlbp-nav.html %}
                     <main role="main" id="page-content">
-                      {% include jlbp-nav.html %}
                       <section id="jump-content">

+              <!-- prettier-ignore -->
+              <footer>
+                {% if site.google_analytics %}
+                <p>This site uses cookies from Google to deliver its services and to analyze traffic. (<a href='https://policies.google.com/technologies/cookies'>learn more</a>)</p>

+                    <!-- LOCK up-->
+                    <div class="glue-header__container">
+                      <div class="glue-header__lock-up">
+                        <div class="glue-header__hamburger glue-header__hamburger--first-tier">

+                border: 1px solid $glue-blue-200;
+              }
+              .tldr {

+                            <p class="glue-header__logo--product">JLBPs</p>
+                          </a>
+                        </div>
+                        <a href="#page-content" class="glue-header__skip-content">

		@@ -0,0 +1,9 @@
		<!-- Closing scripts -->

		@@ -0,0 +1,139 @@
		// This is forked. Do not edit.

Adding a navigational structure based on aip.dev by garrettjonesgoogle · Pull Request #1253 · GoogleCloudPlatform/cloud-opensource-java · GitHub

Navigation Menu

Uh oh!

Adding a navigational structure based on aip.dev - #1253

Adding a navigational structure based on aip.dev#1253garrettjonesgoogle wants to merge 24 commits intoGoogleCloudPlatform:masterGoogleCloudPlatform/cloud-opensource-java:masterfrom garrettjonesgoogle:mastergarrettjonesgoogle/cloud-opensource-java:masterCopy head branch name to clipboard

Conversation

garrettjonesgoogle commented Feb 26, 2020 • edited by elharo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garrettjonesgoogle commented Feb 26, 2020

Uh oh!

suztomo commented Feb 26, 2020

Uh oh!

suztomo commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

garrettjonesgoogle commented Feb 27, 2020

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

garrettjonesgoogle commented Feb 28, 2020

Uh oh!

saturnism commented Feb 28, 2020

Uh oh!

garrettjonesgoogle commented Feb 28, 2020

Uh oh!

saturnism commented Feb 28, 2020

Uh oh!

garrettjonesgoogle commented Feb 28, 2020

Uh oh!

garrettjonesgoogle commented Feb 28, 2020

Uh oh!

garrettjonesgoogle commented Feb 29, 2020

Uh oh!

garrettjonesgoogle commented Feb 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrettjonesgoogle Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adding a navigational structure based on aip.dev#1253
garrettjonesgoogle wants to merge 24 commits into
GoogleCloudPlatform:masterfrom
garrettjonesgoogle:master

garrettjonesgoogle commented Feb 26, 2020 •
edited by elharo
Loading

suztomo commented Feb 26, 2020 •
edited
Loading

garrettjonesgoogle Mar 3, 2020 •
edited
Loading