Skip to content

Added time taken feature #72

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

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

Conversation

vr-varad
Copy link
Contributor

@vr-varad vr-varad commented Jul 31, 2025

Notes for Reviewers

This PR adds reading time to all the levels.

  1. The readtime is sent by JSON data as reading_time in minutes.
image
  1. The readtime is showcased on the page level as:
image 3. The readtime is also being showcased in the module level and course level, but doesn't make sense because there is only metadata and no content, so it will be zero showing `less than a minute` image

We could use hide_readingtime: true in the metadata so that the read time is not shown on these pages.
This change could be made on the archetype as well.

Signed commits

  • Yes, I signed my commits.

@vr-varad vr-varad marked this pull request as draft July 31, 2025 19:53
@vr-varad vr-varad requested a review from zihanKuang July 31, 2025 19:53
@vr-varad vr-varad marked this pull request as ready for review July 31, 2025 20:05
Copy link
Contributor

@zihanKuang zihanKuang left a comment

Choose a reason for hiding this comment

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

Hi Varad😊,
I think the terms "module" and "course" should be considered based on their usage.
For our Layer5 docs' overall section pages, even though they primarily act as navigation, they still include introductory content and explanations. In this case, showing a time estimate seems reasonable.
Also, it makes perfect sense that there's no time displayed for the "knowledge check" section.

However, if we only intend to use "course" and "module" as simple hierarchical distinctions, then we could remove the time display for these types.
By the way, it looks like the CSS for the time display is broken here.

image

@zihanKuang
Copy link
Contributor

There's another question I'm not sure about it: when the site actually renders, does it use the Hugo YAML files from the theme repository or from the build repository? If it's the build repository, then we'd need to modify those files in the build repository, or is there some way to synchronize them?

@vr-varad
Copy link
Contributor Author

vr-varad commented Aug 1, 2025

There's another question I'm not sure about it: when the site actually renders, does it use the Hugo YAML files from the theme repository or from the build repository? If it's the build repository, then we'd need to modify those files in the build repository, or is there some way to synchronize them?

That is why I am sending it over JSON so it can be used as required @zihanKuang

vr-varad and others added 2 commits August 1, 2025 13:22
@vr-varad vr-varad requested a review from zihanKuang August 1, 2025 07:58
Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

@vr-varad this doesn’t exactly answer @zihanKuang questions or fully deliver the feature.

Merge conflict.

@vr-varad
Copy link
Contributor Author

vr-varad commented Aug 1, 2025

@zihanKuang
For academies, I tested it locally using the same changes mentioned above with this repo. It worked fine with just the changes made here in the theme, and I didn't need to make any changes elsewhere.
And I will be making the CSS changes required as well for "module" and "courses"

@leecalcote
Copy link
Member

@vr-varad, offer a screenshot when the CSS is complete.

@vr-varad
Copy link
Contributor Author

vr-varad commented Aug 4, 2025

image image @zihanKuang

Copy link
Contributor

@zihanKuang zihanKuang left a comment

Choose a reason for hiding this comment

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

Hi @vr-varad, what do you think about moving it above? I feel like since 'time' and the other tags aren't aligned anyway, we might as well move them outside the frame.🤔

image

Signed-off-by: vr-varad <[email protected]>
@vr-varad
Copy link
Contributor Author

vr-varad commented Aug 4, 2025

Made the time taken above the tags and categories.
Also only visible on type module and page.
@zihanKuang

@vr-varad vr-varad requested a review from zihanKuang August 4, 2025 10:45
Copy link
Contributor

@zihanKuang zihanKuang left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants