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

USWDS - README: Add note about incorrect package link to download page #5871

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Apr 17, 2024

Summary

Added note to the USWDS README. This will also update the Documentation page on site.

Note

I used HTML instead of standard markdown here in order to utilize the site-note class for the website rendered version of this information:

image

This change was reverted to avoid using HTML within markup

Note now displays on site without the note styles

image

Breaking change

This is not a breaking change.

Related issue

Closes #5872

Related pull requests

uswds/uswds-site#2621

Preview link

Rich content README →

Demo site with documentation preview →

Tip

The demo repo pulls in the readme data of this branch in the site _config.yml [INSERT LINK] file.
I was required to run rm -rf .jekyll_get_cache to clear the jekyll cache and get the documentation page to update.

Problem statement

NPM purposefully sets package dates to 10/26/1985 to ensure hash-identical tarballs no matter when or on what device the npm pack is ran. This can confuse users who inspect the package and see the incorrect files for the date.

Solution

Add note to site to specify this is intended behavior from NPM.

Testing and review

  1. Confirm README has been updated.
  2. Confirm the markup is rendered without HTML tags
  3. Visit the Documentation page of the demo repo and confirm the site note is showing up appropriately
    • I was unable to add the site-note class without it appearing incorrectly in the readme. Opted to match other notes on that page by bolding the Note: span.
  4. Confirm there are no spelling or grammatical errors
  5. Confirm that there is value by linking to the npm decision dates.

Additional context

I added a link to the npm decision on dates that is captured in a GitHub issue. This appears to be the best source of truth for this information. I was unable to find any documentation on their website addressing this concern.

README.md Outdated
@@ -214,6 +214,7 @@ If you’re using a framework or package manager that doesn’t support npm, you

You'll notice in our example above that we also outline a `stylesheets`, `images` and `javascript` folder in your `assets` folder. These folders are to help organize any assets that are unique to your project and separate from the design system assets.

**Note:** The file date defaults to October 26, 1985, when you download and install this package. This is intentional. NPM sets a default date to ensure reproducible builds for npm packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, does this only happen everywhere (NPM, GitHub, Website) or just a specific instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like installing the uswds package using npm will show the last modified date of the current date and time.

This is related solely to the .tgz compressed package

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 the placement of the note in the "Downloading the package from github" and the new wording is enough specificity to now add any additional instruction.

"Files in the downloadable USWDS package..."

Would love to hear your thoughts on if we need to specify further!

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

@mahoneycm
Thanks for setting this up. I added a few questions in the comments below. Let me know if you have any questions!

Also, what do you think about adding this to 3.8.1?

README.md Outdated
@@ -214,6 +214,7 @@ If you’re using a framework or package manager that doesn’t support npm, you

You'll notice in our example above that we also outline a `stylesheets`, `images` and `javascript` folder in your `assets` folder. These folders are to help organize any assets that are unique to your project and separate from the design system assets.

**Note:** The file date defaults to October 26, 1985, when you download and install this package. This is intentional. NPM sets a default date to ensure reproducible builds for npm packages.
Copy link
Contributor

@amyleadem amyleadem Apr 17, 2024

Choose a reason for hiding this comment

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

In the PR description, you mentioned that this note would receive the site-note class. Was it intentionally left off here? Ultimately it depends on how much attention we want on this note, but I lean towards highlighting it with site-note just because the documentation page is so gray otherwise and it would be good to call out notes.

Copy link
Contributor Author

@mahoneycm mahoneycm Apr 17, 2024

Choose a reason for hiding this comment

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

Sorry, originally it had the site-note class. I was under the impression that the readme would be generated without the {:.site-note} liquid block but I was mistaken. I added a note to the testing instructions but forgot to remove it from the summary

I was unable to add the site-note class without it appearing incorrectly in the readme. Opted to match other notes on that page by bolding the Note: span.

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 don't know why I hadn't considered... I am able to add HTML syntax and accomplish this

    <p class="site-note">
      <strong>Note:</strong> Files in the downloadable USWDS package will show a "last modified" date of October 26, 1985. This is <a href="https://github.com/npm/npm/issues/20439#issuecomment-385121133">intentional</a>. This default date is set by npm on all its packages to ensure builds will be identical.
    </p>

Testing for regression 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.

Alright I was able to get this working! Let me know if you have any issues with implementing this way!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this change after @mejiaj mentioned not using HTML within markdown if possible

We should avoid HTML inside of markdown if possible. We just want the content.

README.md Outdated Show resolved Hide resolved
Copy link

@sarah-sch sarah-sch left a comment

Choose a reason for hiding this comment

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

Recommend using Amy L.'s rewrite for plain language. Use "Npm" at the beginning of a sentence (capital N).

README.md Outdated Show resolved Hide resolved
Copy link

@finekatie finekatie left a comment

Choose a reason for hiding this comment

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

looks good!

Copy link

@sarah-sch sarah-sch left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Question on a story change, otherwise LGTM.

packages/usa-form/src/usa-form.stories.js Outdated Show resolved Hide resolved
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Looks good to me! Confirmed it matches the text in uswds/uswds-site#2621

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

Successfully merging this pull request may close these issues.

USWDS - Documentation: Add incorrect date note to README for site Documentation page
5 participants