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

Add dynamic date on blog pages #5801

Merged
merged 9 commits into from
Jan 17, 2024

Conversation

prushh
Copy link
Contributor

@prushh prushh commented Dec 23, 2023

Fixes #5704

Proposed Changes

Additional Info

Warning_NavPluginOrder

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2023
@knative-prow knative-prow bot requested review from nainaz and skonto December 23, 2023 18:57
Copy link

netlify bot commented Dec 23, 2023

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 57c719a
🔍 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.

@knative-prow knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 23, 2023
@Cali0707 Cali0707 requested a review from Leo6Leo January 2, 2024 14:12
@Cali0707
Copy link
Member

Cali0707 commented Jan 2, 2024

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

prushh commented Jan 2, 2024

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

Cali0707 commented Jan 2, 2024

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

Leo6Leo commented Jan 2, 2024

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

prushh commented Jan 2, 2024

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

@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 2, 2024

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

@prushh
Copy link
Contributor Author

prushh commented Jan 4, 2024

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

  • overrides/partials/source-file.html (see link)
  • overrides/partials/content.html (see link)
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
Copy link
Member

Cali0707 commented Jan 4, 2024

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

Leo6Leo commented Jan 4, 2024

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

prushh commented Jan 4, 2024

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

Leo6Leo commented Jan 4, 2024

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

Cali0707 commented Jan 5, 2024

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

show hack using custom theme
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 6, 2024
@prushh
Copy link
Contributor Author

prushh commented Jan 6, 2024

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.

Copy link
Member

@Leo6Leo Leo6Leo left a comment

Choose a reason for hiding this comment

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

And for the issue with this RabbitMQ article you mentioned (have 2 h1 tags), I am guessing the issue might be there are 2 # tags in the article? But not sure.[Link]

@@ -0,0 +1,14 @@
{% if page.meta.git_creation_date_localized and page.meta.git_revision_date_localized %}
Copy link
Member

Choose a reason for hiding this comment

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

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?

</style>


{% if page.toc %}
Copy link
Member

Choose a reason for hiding this comment

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

I personally think this approach is okay for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where should I move the styling? Is it okay to create the new /overrides/assets/stylesheets/content.css file to avoid problems with docs website?

Copy link
Member

Choose a reason for hiding this comment

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

If you think that's a good idea and can explain the reason why this change is worthy, why not? :))

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 think separating CSS from the HTML template is a good practice, as is separating CSS rules based on the files in which they act. For this reason, I will create the file /overrides/assets/stylesheets/content.css that is used only by the /overrides/partials/content.html file and doesn't interact with the docs website 😄 I will also remove the date from blog posts in which it is featured!

@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 8, 2024

[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

@knative-prow knative-prow bot requested a review from a team January 8, 2024 16:45
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 9, 2024
@prushh prushh marked this pull request as ready for review January 9, 2024 17:37
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2024
@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 12, 2024

@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

@knative-prow knative-prow bot requested a review from aliok January 12, 2024 20:27
@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2024
@evankanderson
Copy link
Member

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

Copy link

knative-prow bot commented Jan 16, 2024

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2024
@Cali0707
Copy link
Member

/unhold

Thanks for all of your work on this @prushh 🎉 !!

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2024
@Cali0707
Copy link
Member

Cali0707 commented Jan 16, 2024

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

prushh commented Jan 16, 2024

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

Leo6Leo commented Jan 17, 2024

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

Show always publication date and when a file has been updated, the revision date as well
@prushh
Copy link
Contributor Author

prushh commented Jan 17, 2024

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

@Cali0707
Copy link
Member

Great, thanks @prushh, LGTM from my end.

/cc @Leo6Leo for a final review

@knative-prow knative-prow bot requested a review from Leo6Leo January 17, 2024 14:52
Copy link

knative-prow bot commented Jan 17, 2024

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

Great, thanks @prushh, LGTM from my end.

/cc @Leo6Leo for a final review

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 17, 2024

/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!

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2024
@knative-prow knative-prow bot merged commit 639355d into knative:main Jan 17, 2024
19 checks passed
prushh added a commit to prushh/docs that referenced this pull request Apr 30, 2024
* fix NavPluginOrder warning on docs local preview

* show non-hardcoded date on blog posts

* exclude date from blog homepage

* add dates to all blog post

show hack using custom theme

* fix titles on steering committee section

* show only revised or published date, remove styling from template

* delete hard-coded dates, fix RabbitMQ article

* link content.css stylesheet

* fixup on revised and published date

Show always publication date and when a file has been updated, the revision date as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dates on blog posts
4 participants