knative / docs

User documentation for Knative components.
https://knative.dev/docs/
Other
4.5k stars 1.23k forks source link

Add dynamic date on blog pages #5801

Closed prushh closed 8 months ago

prushh commented 9 months ago

Fixes #5704

Proposed Changes

Additional Info

Warning_NavPluginOrder
netlify[bot] commented 9 months ago

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
Latest commit 57c719a209d3c082f92a29ed243a78044185455c
Latest deploy log https://app.netlify.com/sites/knative/deploys/65a78bf108e5f900083331e3
Deploy Preview https://deploy-preview-5801--knative.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Cali0707 commented 8 months ago

@prushh thanks for your work on this so far! Would you be able to try to use the environment variables described here to populate the Date field at the top of the blog posts?

prushh commented 8 months ago

Would you be able to try to use the environment variables described here to populate the Date field at the top of the blog posts?

@Cali0707 Sure, I'd already tried those environment variables and they work. Which one should I use? The creation date or the revision date?

Is it okay to leave the two labels below when the date is already specified after the blog post title?

Cali0707 commented 8 months ago

@prushh I would lean towards something along the lines of: Date: xxxx-xx-xx, revised: xxxx-xx-xx.

@Leo6Leo WDYT?

In terms of leaving the two labels at the bottom, if there is an easy way to remove them then I think that would be preferrable, but I think it is totally fine to leave those if not.

Leo6Leo commented 8 months ago

Hey @prushh , thanks for the PR! Agreed with what @Cali0707 has mentioned. It would be better to put the date as Date: xxxx-xx-xx, revised: xxxx-xx-xx. Currently with the default preset option for mkdocs-git-revision-date-localized-plugin is with icons.

image

The ideal condition would be something like this:

image

prushh commented 8 months ago

Hi @Leo6Leo, should the ideal condition be the same for all sections (Releases, Articles, etc.)?

Leo6Leo commented 8 months ago

@prushh Yes, let's make all of them have the dates

prushh commented 8 months ago

Hey @Cali0707 @Leo6Leo, I was able to achieve this by customizing the theme. In particular, I add the following files to the blog site:

Schermata 2024-01-04 alle 09 26 08

Due to the title that is included in the page.content, I don't know how to put the dates after it. Some suggestions?


Another option should be add the following string by hand on each blog post, and have an empty source-file.html file to avoid bottom icons:

**Published on: {{git_creation_date_localized}}, Revised on: {{git_revision_date_localized}}**

Schermata 2024-01-04 alle 09 29 39

I think we can change the style in both cases.

Cali0707 commented 8 months ago

Another option should be add the following string by hand on each blog post, and have an empty source-file.html file to avoid bottom icons:

Personally I think that this looks better, and as we are already adding the blog post dates manually I don't think it's a big ask for new blog posts to include those variables. WDYT @Leo6Leo (also cc @mmejia02 @zainabhusain227 as our UX design leads)

Leo6Leo commented 8 months ago

I prefer the second option too, but this could lead to inconsistencies between old and new posts if occasionally overlooked.

If it's not feasible to automate this process by modifying overrides/partials/source-file.html and overrides/partials/content.html, we'll need to opt for the manual approach.

To ensure consistency, we can create a standard blog post template with these variables and integrate a CI test to check for their inclusion in new posts, and implement a system of reminders for content creators, possibly through documentation or a checklist, emphasizing the importance of including these variables. This should help maintain uniformity across our content!

prushh commented 8 months ago

I may have found an hack to solve the problem using the first option! I added to the overrides/partials/content.html file a new h1 selector that contains the title, I declared it before the overrides/partials/source-file.html inclusion, that contains the date labels. After, I used CSS to hide the second h1 selector in the page.

Schermata 2024-01-04 alle 18 58 59

I know it isn't a pretty solution, but it works 🤔 I should find a way to retrieve id and href for the current page, so as to update TODOs.

Leo6Leo commented 8 months ago

That's awesome to hear! I think it is okay if the solution isn't too pretty, as long as it can work perfectly. As the Knative UX WG are currently planning to redesign the whole website, so the infrastructure of the website might need to be changed in the future. @Cali0707 WDYT?

Cali0707 commented 8 months ago

@prushh I like the screenshot of what you have there. Do you think you could push another commit to this PR with the changes so that we can take a closer look at if it is too hacky or not?

prushh commented 8 months ago

Any suggestions would be appreciated, for now I left the styling inside the overrides/partials/content.html file. I noticed issues when the title is defined inside an h2 tag (see v1.5 release).

Also in Steering Committee section the posts have problems: there isn't any title because in there I should use page.title variable instead of toc (fixed).

In Announcing Eventing RabbitMQ moving to GA article there are two h1 header. I don't understand why page.toc[0].title and page.toc[0].url didn't work, I must use the for statement to retrieve those data.

Leo6Leo commented 8 months ago

[Note] And just a note, this PR relates to the UI change in the knative.dev website, will need approval from Knative Steering Committee.

/cc @knative/steering-committee

Leo6Leo commented 8 months ago

@prushh Just share some update: this approval process takes some time and discussion, so thanks for your patience :) I will keep you posted if there are any updates! Feel free to let me know if you have any questions or concern.

/cc @aliok

/hold

evankanderson commented 8 months ago

/approve

I think minor changes like this should be up to the approvers for the docs repo, particularly the OWNERS for the directories that contain the styling. For large-scale changes ("our new theme colors are hot pink and orange!"), Steering would like the option to review before they go live. Smaller fixes like date & time on blog posts or accessibility fixes should be up to y'all's discretion.

knative-prow[bot] commented 8 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, prushh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/knative/docs/blob/main/OWNERS)~~ [evankanderson] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
Cali0707 commented 8 months ago

/unhold

Thanks for all of your work on this @prushh :tada: !!

Cali0707 commented 8 months ago

@prushh it looks like only the Revised-on date is showing up currently, never the published date (as far as I can tell), any ideas why that is the case?

prushh commented 8 months ago

@Cali0707 I followed Leo's suggestion, do you think is better to leave both?

In some posts (e.g. Announcing Knative 1.11 Release, TOC 2022 election announcement, etc.) where no changes have been made the Published-on date is shown correctly.

Leo6Leo commented 8 months ago

Quick suggestion: Let's only show 'Revised on' for posts that have been updated. If it's not edited since posting, we'll skip this part to keep things clear and simple. How does that sound?

@Cali0707 I followed Leo's suggestion, do you think is better to leave both?

In some posts (e.g. Announcing Knative 1.11 Release, TOC 2022 election announcement, etc.) where no changes have been made the Published-on date is shown correctly.

@prushh Thanks for the PR and great suggestions you have proposed!

Just to clarify my comment: I mean we should always show Publised on, and show Revised on at the same time if the post has been updated since published.

For example: Article 1 is created on 2024-1-1, and it has been modified once on 2024-1-3 The page should show: Published on 2024-1-1 Revised on 2024-1-3

Article 2 is created on 2024-1-1, but it hasn't been modified since created The page should show this only: Published on 2024-1-1

I hope this is clear! Lmk if there are anything confusing still

prushh commented 8 months ago

@Leo6Leo My fault, sorry for the misunderstanding! Now should be everything okay 😄

Cali0707 commented 8 months ago

Great, thanks @prushh, LGTM from my end.

/cc @Leo6Leo for a final review

knative-prow[bot] commented 8 months ago

@Cali0707: GitHub didn't allow me to request PR reviews from the following users: review, for, a, final.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/knative/docs/pull/5801#issuecomment-1895974422): >Great, thanks @prushh, LGTM from my end. > >/cc @Leo6Leo for a final review Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Leo6Leo commented 8 months ago

/lgtm

This looks good! @prushh Thank you for the contribution! Hope you have learnt something here and we are looking forward to seeing more contribution from you in the knative community Davide!