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

Provide a per-page outdated notification for localized content #41768

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

morix1500
Copy link
Contributor

@morix1500 morix1500 commented Jun 26, 2023

I have implemented a notification on the localization page to alert users that information may be out of date.
issue: #41519

This function is implemented with the following logic

  1. get the Lastmod of the currently opened page.
  2. get the Lastmod of the English version of the page.
  3. compare 1 and 2 and display a notification if 1 is older.

I am a Japanese speaker and could not configure non-English i18n settings.
If this implementation details are ok, I would appreciate it if you could work with me to come up with messages in other languages :)

DaemonSet._.Kubernetes.mp4

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2023
@k8s-ci-robot k8s-ci-robot requested a review from atoato88 June 26, 2023 12:02
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jun 26, 2023
@k8s-ci-robot k8s-ci-robot requested a review from onlydole June 26, 2023 12:02
@k8s-ci-robot k8s-ci-robot added language/ja Issues or PRs related to Japanese language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2023
@netlify
Copy link

netlify bot commented Jun 26, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 4fa23bc
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/64a3f819fa48dd0008eaa888
😎 Deploy Preview https://deploy-preview-41768--kubernetes-io-main-staging.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.

@sftim
Copy link
Contributor

sftim commented Jun 30, 2023

/area web-development

@k8s-ci-robot k8s-ci-robot added the area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes label Jun 30, 2023
Copy link
Contributor

@sftim sftim 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! It's nice to see this.

Almost LGTM. Can we use a less strong marker than danger though?

layouts/partials/docs/outdated_content.html Outdated Show resolved Hide resolved
data/i18n/en/en.toml Outdated Show resolved Hide resolved
@sftim
Copy link
Contributor

sftim commented Jul 2, 2023

LGTM
(this PR implements what its description says it does)

However:
Please omit the Japanese localization for the added strings. That should arrive in a follow up PR.

Once you make that change @morix1500, I recommend you squash this down to 1 commit.

@morix1500 morix1500 force-pushed the outdated_content_caution branch from 7a7e536 to a10fce9 Compare July 3, 2023 03:23
@sftim
Copy link
Contributor

sftim commented Jul 3, 2023

As this stands:
/lgtm

The tweak I suggested in #41768 (review) is welcome too; either as a change in this PR, or as a follow up.

Thank you @morix1500

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 36bcd3dc860ff03cff45388dd78c147d231d9c94

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023
@k8s-ci-robot k8s-ci-robot requested a review from sftim July 3, 2023 11:08
@sftim
Copy link
Contributor

sftim commented Jul 3, 2023

/lgtm

This PR does what the description says it should.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1d4329e9c871d485ddc10d1ce19a63c062ffed17

@sftim
Copy link
Contributor

sftim commented Jul 3, 2023

/area localization

@k8s-ci-robot k8s-ci-robot added the area/localization General issues or PRs related to localization label Jul 3, 2023
@anubha-v-ardhan
Copy link
Member

Thank you @morix1500 for this! This will be extremely helpful.

@stormqueen1990
Copy link
Member

This is a nice addition for the website! Thanks for working on this @morix1500 😃

@stormqueen1990
Copy link
Member

I think the files for the Japanese localization were removed but the label wasn't removed.

/remove-language ja

@k8s-ci-robot k8s-ci-robot removed the language/ja Issues or PRs related to Japanese language label Jul 3, 2023
@rolfedh
Copy link
Contributor

rolfedh commented Jul 3, 2023

I believe this is a positive addition and a step in the right direction. However, I have identified a potential limitation. Currently, any update made to the localized file modifies the Lastmod value and disables the "Needs update" notification, even if further content updates are required. It is important to make the localization teams aware of this limitation and encourage them to take additional measures. If feasible, they should consider adopting the "automated-diff" scripts utilized by the Korean localization team. These scripts have proven to be effective in ensuring accurate updates.

@stormqueen1990
Copy link
Member

I believe this is a positive addition and a step in the right direction. However, I have identified a potential limitation that needs to be addressed. Currently, any update made to the localized file modifies the Lastmod value and disables the "Needs update" notification, even if further content updates are required. It is important to make the localization teams aware of this limitation and encourage them to take additional measures. If feasible, they should consider adopting the "automated-diff" scripts utilized by the Korean localization team. These scripts have proven to be effective in ensuring accurate updates.

A suggestion to address this risk would be to adopt a new front-matter field as a next step that indicates the version of a given page. I imagine it might be undesirable from a broader docs perspective, but a change such as that could allow more reliable checks across localized content.

@sftim
Copy link
Contributor

sftim commented Jul 3, 2023

I believe this is a positive addition and a step in the right direction. However, I have identified a potential limitation. Currently, any update made to the localized file modifies the Lastmod value and disables the "Needs update" notification, even if further content updates are required. It is important to make the localization teams aware of this limitation and encourage them to take additional measures. If feasible, they should consider adopting the "automated-diff" scripts utilized by the Korean localization team. These scripts have proven to be effective in ensuring accurate updates.

Is the lack of a notice a problem? If so, how much?

If in future we want to track where a page is up to, I'd record the short ref that the page was last brought fully up to data against. But really we'd love to make localization work simpler, not add effort.

@rolfedh
Copy link
Contributor

rolfedh commented Jul 3, 2023

I love the simplicity of this solution and definitely would not want to make things more complicated. However...

Is the lack of a notice a problem?

Indeed. Users and localization contributors might misinterpret the missing notice as an indicator that the content is up-to-date, leading to the following unintended consequences:

  • Users who need the current recent information (from the unlocalized English content) might use the outdated localized content.
  • Localizers might overlook the need to refresh the localized content.

If so, how much?

This is unclear and varies depending on the circumstances.

  • As it stands, without a notification, users are left to decipher this on their own.
  • By introducing this notification, we take on the liability of providing information that occasionally misleads.

@tengqm
Copy link
Contributor

tengqm commented Jul 3, 2023

Here is how we update a zh-cn localization page. The localization team reached a consensus about two years ago:

  • We check if a page is "fresh" or not based on its last commit time (against the English upstream).
    Nothing is simpler than time comparison.
  • To make timestamp-based checking feasible, the localization reviewers need to
    ensure each time a page is being "refreshed" by a PR,
    all upstream changes to that page are incorporated in the same PR.
    In other words, no partial updates are supposed to be merged.

These rules appeared too strict during the first few weeks/months. When the convention
is formed and shared across the team, everything works great till now.

There is a secret tool which really helped, scripts/lsync.sh.
To check if a ja page is out-dated, run:

./scripts/lsync.sh content/ja/docs/path/to/page.md

To get an overview of out-dated pages, run:

./scripts/lsync.sh content/ja/docs/path/to/directory/

@tengqm
Copy link
Contributor

tengqm commented Jul 3, 2023

As a side note, I'm not a big fan of adding more banners to the pages.
I've just filed an issue related to this. #41859

@atoato88
Copy link
Contributor

atoato88 commented Jul 4, 2023

@morix1500
As commented in previous review, you can squash the commits.
Please see this ja page or en page.

@morix1500 morix1500 force-pushed the outdated_content_caution branch from e688b92 to 7c0d55e Compare July 4, 2023 03:28
@shurup
Copy link
Member

shurup commented Jul 4, 2023

Love this change and really happy to see how fast the idea was implemented after our recent discussion in Slack. While it is a simple implementation, I fully support it. I think we can think about improving it further after introducing this change to the end users and getting their first feedback.

It will be also important to inform all localisation teams about this change and its limitations (the possible Lastmod-based-only inconsistency concern that was fairly mentioned above).

@morix1500 morix1500 force-pushed the outdated_content_caution branch from 7c0d55e to 4fa23bc Compare July 4, 2023 10:44
@reylejano
Copy link
Member

Preview of the change proposed in this PR
image
I support this change
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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

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. area/localization General issues or PRs related to localization area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants