Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Adding a navigational structure based on aip.dev #1253

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

garrettjonesgoogle
Copy link
Contributor

@garrettjonesgoogle garrettjonesgoogle commented Feb 26, 2020

  • 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

@garrettjonesgoogle
Copy link
Contributor Author

@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

@suztomo
Copy link
Contributor

suztomo commented Feb 26, 2020

Checking out your branch...

@suztomo
Copy link
Contributor

suztomo commented Feb 26, 2020

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.

image

I didn't find any missing link.

<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elharo what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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.

@garrettjonesgoogle
Copy link
Contributor Author

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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

@garrettjonesgoogle
Copy link
Contributor Author

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.

@saturnism
Copy link

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

@garrettjonesgoogle
Copy link
Contributor Author

@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
Copy link

@saturnism :

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.

@garrettjonesgoogle
Copy link
Contributor Author

@saturnism :

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 .

@garrettjonesgoogle
Copy link
Contributor Author

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

@garrettjonesgoogle
Copy link
Contributor Author

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

@garrettjonesgoogle
Copy link
Contributor Author

Screenshot from 2020-02-28 16-11-05

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

Choose a reason for hiding this comment

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

What does this script do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@garrettjonesgoogle garrettjonesgoogle Mar 3, 2020

Choose a reason for hiding this comment

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

Offline answer:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add comments to the file explaining this.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert this line.

border: 1px solid $glue-blue-200;
}

.tldr {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

docs/assets/images/glue-icons.svg Show resolved Hide resolved
@garrettjonesgoogle
Copy link
Contributor Author

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

@garrettjonesgoogle
Copy link
Contributor Author

Could the View on Github link go straight to the docs directory instead of the main page of the project?

Will do.

@garrettjonesgoogle
Copy link
Contributor Author

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.

@garrettjonesgoogle
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@garrettjonesgoogle
Copy link
Contributor Author

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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:

image

@garrettjonesgoogle
Copy link
Contributor Author

Done - Footer updated to black.

@garrettjonesgoogle
Copy link
Contributor Author

@elharo what browser are you using to check accessibility?

@elharo
Copy link
Contributor

elharo commented Mar 6, 2020 via email

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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>

@garrettjonesgoogle
Copy link
Contributor Author

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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.

image

@garrettjonesgoogle
Copy link
Contributor Author

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 .

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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

image

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.

<!-- 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>
Copy link
Contributor

Choose a reason for hiding this comment

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

learn --> Learn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@garrettjonesgoogle
Copy link
Contributor Author

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:

  1. It is the conventional layout across Google sites to include a header
  2. 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
  3. The header provides a "jump to content" accessibility link (part of Glue recommendations)
  4. 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.
  5. Stripping the header from this PR but then adding it in a following PR creates a bit more unnecessary work

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Firefox reports four remaining accessibility issues.

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

<div class="glue-header__lock-up">
<div class="glue-header__hamburger glue-header__hamburger--first-tier">
<div class="glue-header__hamburger-wrapper">
<button type="button" class="glue-header__drawer-toggle-btn"
Copy link
Contributor

Choose a reason for hiding this comment

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

"Warning

Focusable elements should have interactive semantics."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is taken straight from the Glue sample:

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

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

Choose a reason for hiding this comment

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

"Error

Interactive elements must be labeled. Learn more"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

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.

<p class="glue-header__logo--product">JLBPs</p>
</a>
</div>
<a href="#page-content" class="glue-header__skip-content">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<!-- LOCK up-->
<div class="glue-header__container">
<div class="glue-header__lock-up">
<div class="glue-header__hamburger glue-header__hamburger--first-tier">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

<div class="glue-header__lock-up">
<div class="glue-header__hamburger glue-header__hamburger--first-tier">
<div class="glue-header__hamburger-wrapper">
<button type="button" class="glue-header__drawer-toggle-btn"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is taken straight from the Glue sample:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Glue Previous | Home | Next links at bottom of JLBP pages Navigation sidebar in JLBP
5 participants