feat(sigs): dynamic grid layout and content adapters modernization#654
feat(sigs): dynamic grid layout and content adapters modernization#654TineoC wants to merge 10 commits intokubernetes:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TineoC The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
While this is a nice feature to add, it significantly adds to our build times, especially at the gen-content step. We could wait till this is merged so we can add these changes after. |
|
/hold /label draft |
I would like to hear your thoughts on this idea @kaslin @chris-short |
|
/remove-hold |
|
If you didn't also consider a content adapter to render one page per SIG, then bear in mind that that is also an option. I quite like the two-column layout, though. |
|
/hold |
…ject This addresses kubernetes#653 and prepares for kubernetes#654
02785dc to
9d392a0
Compare
9d392a0 to
49296d4
Compare
… adapters and SEO improvements
49296d4 to
3b3b615
Compare
|
/unhold |
lmktfy
left a comment
There was a problem hiding this comment.
Thanks! It's impressive.
I can spot a lot of minor problems, but actually the main thing that gets in the way of adding LGTM is the mobile-viewport rendering.
See this video to illustrate what is wrong:
Scroll behavior
I think we should improve that before we merge. The other nits and minor feedback, you can fix if you feel you'd like to. I recommend fixing at least the alias clash.
.gitignore
Outdated
| # Ingested documents | ||
| data/sigs.yaml |
There was a problem hiding this comment.
Is this needed? I think it no longer is.
content/en/community/.gitignore
Outdated
| mentoring.md | ||
| values.md | ||
| sigs.yaml | ||
| community-groups/** |
layouts/partials/head.html
Outdated
| <link rel="{{ .Rel }}" type="{{ .MediaType.Type }}" href="{{ .Permalink | safeURL }}"> | ||
| {{ end -}} | ||
|
|
||
| <link rel="canonical" href="{{ .Permalink }}"> |
There was a problem hiding this comment.
Also, could it go in layouts/partials/hooks/head-end.html instead? That's the usual home for custom additions.
There was a problem hiding this comment.
Why this change?
This is for SEO to make sure that no matter the version of the sub-page they're visiting, for the search engine, it is the same content.
So it doesn't treat these like different pages, like for example:
1. https://contributor.kubernetes.io/blog/my-post/ (The "official" URL)
2. https://contributor.kubernetes.io/blog/my-post (No trailing slash)
3. http://contributor.kubernetes.io/blog/my-post/ (Insecure version)
4. https://contributor.kubernetes.io/blog/my-post/?utm_source=twitter (With
tracking parameters)
It would look like: <link rel="canonical" href="https://contributor.kubernetes.io/blog/my-post/">
There was a problem hiding this comment.
Will this overwrite the canonicalUrl defined in blog posts' front matter, where k8s.io or etcd.io is the canonical? My testing shows this breaks that. I ask because I'd like this to be future-proof. We may want something more flexible, like:
<link rel="canonical" href="{{ with .Params.canonicalURL }}{{ . }}{{ else }}{{ .Permalink }}{{ end }}">FWIW, k8s.io is using this currently, which would also work:
{{ with .Params.canonicalUrl }}
<link rel="canonical" href="{{ . }}">
{{ end }}There was a problem hiding this comment.
c92cbea This should allow to override those params
There was a problem hiding this comment.
This appears to be working as desired. Thank you, @TineoC!
layouts/shortcodes/sigs-list.html
Outdated
| {{/* layouts/shortcodes/sigs-list.html */}} | ||
|
|
||
| {{- $showOnly := .Get "show_only" | default "all" -}} | ||
| {{- $sigsUrl := "https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml" -}} |
There was a problem hiding this comment.
nit: we might want to give a GitHub token to Netlify to ensure that production builds are authenticated and aren't rate limited.
layouts/shortcodes/sigs-list.html
Outdated
| </div> | ||
| </div> | ||
|
|
||
| <script> |
There was a problem hiding this comment.
nit: I'd put this script inside assets/js and load it at site build time.
There was a problem hiding this comment.
(can use layouts/partials/hooks/head-end.html for that, if we want)
Want to load a script conditionally? .HasShortcode helps.
layouts/shortcodes/sigs-list.html
Outdated
| <h5 class="fw-bold mb-0 text-primary h6"> | ||
| <a href="/community/community-groups/wg/{{ .dir | replaceRE "^sig-" "" | replaceRE "^wg-" "" }}/" class="text-decoration-none text-primary">WG {{ .name }}</a> | ||
| </h5> | ||
| <a href="https://github.com/kubernetes/community/tree/master/{{ .dir }}" class="text-muted" title="View Source"><i class="fab fa-github"></i></a> |
There was a problem hiding this comment.
nit: extract the name of the default branch, master, to a variable at the top of this shortcode file.
layouts/shortcodes/sigs-list.html
Outdated
| <h5 class="fw-bold mb-0 text-primary h6"> | ||
| <a href="/community/community-groups/committees/{{ .dir | replaceRE "^sig-" "" | replaceRE "^committee-" "" }}/" class="text-decoration-none text-primary">{{ .name }}</a> | ||
| </h5> | ||
| <a href="https://github.com/kubernetes/community/tree/master/{{ .dir }}" class="text-muted" title="View Source"><i class="fab fa-github"></i></a> |
There was a problem hiding this comment.
nit: extract the name of the default branch, master, to a variable at the top of this shortcode file.
| @@ -0,0 +1,168 @@ | |||
| {{/* content/en/community/community-groups/_content.gotmpl */}} | |||
|
|
|||
| {{ $sigsUrl := "https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml" }} | |||
There was a problem hiding this comment.
nit: extract the name of the default branch, master, to a separate variable.
layouts/shortcodes/sigs-list.html
Outdated
| tag.classList.replace('text-primary', 'text-white'); | ||
| } else { | ||
| tag.classList.replace('bg-opacity-100', 'bg-opacity-10'); | ||
| tag.classList.replace('text-white', 'text-primary'); |
There was a problem hiding this comment.
nit: avoid using presentation classes in this kind of script; it means that if we ever add dark mode, we need some basic JavaScript skills in the mix,
| A list of our community groups: Special Interest Groups, Working Groups and Committees. | ||
| weight: 99 | ||
| type: docs | ||
| aliases: [ "/groups", "/sigs" ] |
There was a problem hiding this comment.
This clashes with aliases: [ "/sigs" ] for content/en/community/community-groups/sigs/_index.md
Avoid having two different pages that both say that a different URL is an alias for themselves.
TineoC
left a comment
There was a problem hiding this comment.
Starting review...
| A list of our community groups: Special Interest Groups, Working Groups and Committees. | ||
| weight: 99 | ||
| type: docs | ||
| aliases: [ "/groups", "/sigs" ] |
There was a problem hiding this comment.
[MEDIUM] The alias /sigs is defined in both this file and content/en/community/community-groups/sigs/_index.md. This will lead to unpredictable redirect behavior. Choose one location (preferably the SIGs index page) for the /sigs alias.
| {{ end }} | ||
| {{ end }} | ||
|
|
||
| {{ $page := dict |
There was a problem hiding this comment.
[HIGH] Fetching remote resources (READMEs and Charters) within a range loop over dozens of groups will significantly increase build times, especially on CI or when the cache is empty. Consider fetching only the main sigs.yaml and using it to generate links, or optimize by minimizing the number of remote calls if possible. Additionally, ensure consistent error handling across SIGs, WGs, and Committees.
There was a problem hiding this comment.
The cache, if we add it, should be enough.
There was a problem hiding this comment.
I've added a getresource configuration to our Hugo.yaml's internal caching block with a maxAge of 2h. Since resources.GetRemote automatically utilizes this cache.
Let me know if you would like to change this.
layouts/shortcodes/sigs-list.html
Outdated
| {{ if eq $showOnly "all" }} | ||
| <!-- Navigation Area --> | ||
| <div class="col-lg-12 mb-3"> | ||
| <div class="nav nav-pills shadow-sm rounded bg-white p-2 border flex-nowrap overflow-auto justify-content-lg-center" id="sigsTab" role="tablist" style="scrollbar-width: none; -ms-overflow-style: none;"> |
There was a problem hiding this comment.
[MEDIUM] The use of scrollbar-width: none; -ms-overflow-style: none; effectively hides the scrollbar across all browsers. While it maintains a clean aesthetic, it can result in a confusing user experience on mobile devices where the horizontal scroll of the nav pills is not immediately apparent to the user.
layouts/shortcodes/sigs-list.html
Outdated
| wrapper.classList.remove('d-none'); | ||
| } else { | ||
| wrapper.classList.add('d-none'); | ||
| } |
There was a problem hiding this comment.
[LOW] The inline script for search and filtering has grown to over 100 lines. To maintain readability and potentially reuse this logic, consider moving the JavaScript to a separate file (e.g., in static/js/) and including it via head-end.html or another partial.
| <div class="input-group input-group-sm border rounded flex-grow-1" style="max-width: 300px;"> | ||
| <span class="input-group-text bg-white border-0"><i class="fas fa-search"></i></span> | ||
| <input type="text" id="sigSearchInput" class="form-control border-0" placeholder="Type name, tag, area..." aria-label="Search"> | ||
| </div> |
There was a problem hiding this comment.
[MEDIUM] The filter bar should include more descriptive ARIA labels to assist users with screen readers. For example, adding aria-label='Filter community groups by name or tag' to the search input and ensuring the topic tags have appropriate roles or labels.
TineoC
left a comment
There was a problem hiding this comment.
📋 Review Summary
The PR successfully modernizes the community groups section with Hugo Content Adapters and a responsive grid layout. However, there are significant performance concerns regarding remote resource fetching and a critical alias clash that needs to be resolved before merging.
🔍 General Feedback
- Performance: The current approach of fetching READMEs and Charters for 50+ groups during the Hugo build will likely lead to slow build times and CI timeouts. Consider linking to external READMEs or optimizing the fetching logic.
- Alias Clash: The
/sigsalias is duplicated, which can cause unpredictable routing. - Mobile UX: The hidden scrollbars on nav pills hinder discoverability of content on smaller screens.
- Accessibility: Enhancing ARIA labels in the filter bar will improve the experience for screen reader users.
Thank you. Please re-attach this video, is giving me 404 |
* Resolve duplicate '/sigs' alias conflict. * Optimize build performance by linking to READMEs/Charters on GitHub instead of remote fetching. * Implement custom layout for SIG/WG pages to display metadata and GitHub links. * Improve accessibility with better ARIA labels in the filter bar. * Enhance mobile UX by removing hidden scrollbars and externalizing JavaScript.
* Remove redundant ignores from .gitignore and community/.gitignore * Move canonical link and script to head-end.html hook * Use generic .active class for topic tags with SCSS styles * Extract master branch variable to reduce hardcoding * Add caching support and optional token for resources.GetRemote * Rename show_only wg to wgs for consistency
|
@lmktfy Do you want to add the token before merging these changes? If so, I would set a hold, and please tag anyone who could help with this. |
* Update content adapter to fetch READMEs and Charters with caching. * Update community layout to prioritize embedded content. * Add navigation link to Charter sub-page if generated.
Not needed IMO. It's worth you knowing that we could, but it isn't required. What happens with the build if the GET returns 429? We should (again, my opinion) aim to fail the whole build rather than publish a partial site. |
…allback to permalink.
…calURL` parameter casing.

This PR modernizes the community groups section by implementing Hugo Content Adapters and a new responsive grid layout.
Key Changes:
Netlify Configuration (Required for Production):
This PR introduces authenticated remote resource fetching to prevent GitHub API rate limiting during site builds.
An environment variable named
HUGO_GITHUB_TOKENmust be added to the Netlify site settings.Fixes: #653