Skip to content

Conversation

@andy-stark-redis
Copy link
Contributor

@andy-stark-redis andy-stark-redis commented May 15, 2025

DOC-5131

I've added a table for the client and tool links and thrown in a few speculative formatting ideas. Also, I've tried removing the page ToC bar from the right-hand side.

BTW, this also fixes something I didn't notice on the previous PR - all section index pages using the list.html layout had the CLI widget at the top (ie, index pages for data types, client APIs, etc). Maybe we should leave that in, actually - I don't know.

Anyway, all (polite) feedback welcome :-)

@andy-stark-redis andy-stark-redis self-assigned this May 15, 2025
@github-actions
Copy link
Contributor

@dwdougherty
Copy link
Collaborator

dwdougherty commented May 15, 2025

GAH! Thank you for catching my mistake, @andy-stark-redis. I've a PR that fixes this up now. Apologies for introducing merge conflicts for this PR!

Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

  1. I'm always polite. 🙂 Aren't I? 🥺
  2. I like the content and image selection. I think it's just the right amount. Michelle might have other ideas though. 😉
  3. Rather than using an entirely new layout to exclude the right side TOC, I'm wondering if you could instead use a bit of frontmatter, say hideTOC: true to hide the TOC as appropriate for each _index.md page. It would work just like the hideListLinks frontmatter. It would also be easier for other folks to use that way, though they would have to make the same changes to the operate list layout as shown below.
{{ if not .Params.hideTOC }}
      {{ partial "docs-toc.html" . }}
{{ end }}
  1. The images don't show in preview (but they do locally 🤔). You might consider using the image shortcode; it's used very widely in the K8s, RC, and RS docs.
  2. If you don't intend on reusing the content elsewhere, I think it's better to "inline" the content currently stored in the two shortcodes. Or maybe consider using embeds, but only if you intend to reuse that content.

Feel free to reach out via Slack if you want to discuss.

@andy-stark-redis
Copy link
Contributor Author

@dwdougherty Thanks for the review! In response:

  1. You are indeed always polite. It was that uncouth ne'er-do-well @mich-elle-luna I was concerned about.
  2. Michelle is a lady of great taste and judgement, so her opinions would be most welcome.
  3. Great idea - implemented.
  4. Strange combination of the relURL function and some missing quotes around the image filename - it works OK locally, but doesn't work in the build. Anyway, fixed now.
  5. The shortcodes were basically just a way to include some raw HTML neatly in the page but still give us a few options for adding stuff in between and around. I guess I could inline the HTML once we're all happy with the look of this page.

Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

You can leave the content shortcodes for now. As for the rest, it looks good. I'll go ahead and approve, but please don't merge until @mich-elle-luna weighs in.

"It was that uncouth ne'er-do-well @mich-elle-luna I was concerned about." LOL! There was art in that sentiment. :)

@dwdougherty
Copy link
Collaborator

It's weird that it doesn't show a merge conflict. As it stands now, were you to merge this PR, it would overwrite a change I made last night to the develop list.html layout.

@andy-stark-redis
Copy link
Contributor Author

It's weird that it doesn't show a merge conflict. As it stands now, were you to merge this PR, it would overwrite a change I made last night to the develop list.html layout.

@dwdougherty I resolved the merge conflict earlier.

@dwdougherty
Copy link
Collaborator

dwdougherty commented May 16, 2025

The file still shows the old test:

<!-- Responsive CLI Display Block as Table -->
{{ if eq .RelPermalink "/develop/" }}

The test should be:

{{ if strings.HasSuffix .RelPermalink "/develop/" }}

Maybe I just don't understand Git/GitHub. I blame dotage. :(

{{ if strings.HasSuffix .RelPermalink "/develop/" }}
<div class="overflow-x-auto">
<table class="w-full border-collapse md:table">
<table class="w-full border-collapse md:table" style="background-color: rgb(240,240,240)">
Copy link
Collaborator

@dwdougherty dwdougherty May 16, 2025

Choose a reason for hiding this comment

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

Please change this line to the following to stick with Tailwind CSS styling.

<table class="w-full border-collapse md:table bg-neutral-150">

BTW, I like the background for this page element; really makes it pop!

@mich-elle-luna
Copy link
Collaborator

I think this is looking really nice! I might suggest that the screenshots for insight and vscode appear in a larger size to break up the page a bit more and highlight those UIs, but I think this is a great update to start with and see how it goes. Thank you!

mich-elle-luna
mich-elle-luna previously approved these changes May 16, 2025
Copy link
Collaborator

@mich-elle-luna mich-elle-luna left a comment

Choose a reason for hiding this comment

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

thank you!

@dwdougherty
Copy link
Collaborator

One final comment: Do you know why the three columns of the last table are not spread out evenly on the page? This is triggering my OCD! 😂

@mich-elle-luna mich-elle-luna dismissed their stale review May 16, 2025 17:45

actually still has a footer problem

@mich-elle-luna
Copy link
Collaborator

I'm not sure if the table column width issue is connected, but the footer should appear all the way across the page. Right now, it is stuck on the right side and needs to be fixed before merging.

Copy link
Collaborator

@mich-elle-luna mich-elle-luna 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 not sure where the problem is, but the footer in the preview is not appearing all the way across the page.

Comment on lines 1 to 3
<table>
<tr>
<td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these three lines, which cause the footer to behave erratically.

</tr>
<tr>
<td>
<ul style="list-style-type: none; padding-left:0%;margin-left:-2%">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace styling with this Tailwind CSS class:

class="list-none pl-0 -ml-[2%]"

Same for the other instances further down.

@dwdougherty
Copy link
Collaborator

dwdougherty commented May 17, 2025

The first three lines of the client-splash.html file were redundant. See code comment above. This is what was messing up the footer.

@andy-stark-redis
Copy link
Contributor Author

@dwdougherty Thanks for your new _index.md content - it fixes all the issues and avoids any remaining need for the two shortcode files, so I've added it now (and deleted the shortcode files).

@mich-elle-luna David's updates fix the footer issue now and also the two screenshots are now a bit bigger. See what you think.

Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

LG(reat)TM!

@andy-stark-redis
Copy link
Contributor Author

@dwdougherty I think we've made a good joint effort here :-) Let's see if @mich-elle-luna has any further suggestions, and if we're all happy then we can merge.

@dwdougherty dwdougherty added enhancement New feature or request and removed do not merge yet labels May 19, 2025
Copy link
Collaborator

@mich-elle-luna mich-elle-luna left a comment

Choose a reason for hiding this comment

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

thank you!

@andy-stark-redis andy-stark-redis merged commit f0f4447 into main May 20, 2025
5 checks passed
@andy-stark-redis andy-stark-redis deleted the DOC-5131-dev-landing-links branch May 20, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants