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

Markdown-driven static pages: building the framework #5601

Merged
merged 22 commits into from
Jun 2, 2022

Conversation

kevinhaube
Copy link
Contributor

@kevinhaube kevinhaube commented May 26, 2022

This PR adds the framework in to create static pages from Markdown files. This is exemplified by adding in a Built For You example and the Admin > Guides page. Both of these pages are generated by the new framework.

Test Steps:

  1. Login as an admin, and add built-for-you to your feature flags by navigating to Admin > Feature Flags
  2. Refresh the page and ensure you can see Built For You. This means it's successfully hidden behind that feature flag and no end user will see it.
  3. Ensure it works as intended.

Changes

  • MarkdownDirectory is an object holding markdown files.
  • StaticPageFromDirectories is a component that takes an array of directories and generates static pages from the markdown content. This also generates side navigation.
  • DropdownNav is a generative way of adding dropdown navigation items to our header.
  • Added the /built-for-you route
  • Added the /admin/guides route
  • Created the creating-markdown-pages internal user guide

Screen Shot 2022-05-31 at 10 51 59 PM

Screen Shot 2022-05-31 at 10 53 39 PM

Checklist

Testing

  • Tested locally?
  • Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
  • (For Changes to /frontend-react/...) Ran npm run lint:write?
  • Added tests?

Process

  • Are there licensing issues with any new dependencies introduced?
  • Includes a summary of what a code reviewer should test/verify?
  • Updated the release notes?
  • Database changes are submitted as a separate PR?
  • DevOps team has been notified if PR requires ops support?

Linked Issues

Specific Security-related subjects a reviewer should pay specific attention to

  • Does this PR introduce new endpoints?
    • new endpoint A
    • new endpoint B
  • Does this PR include changes in authentication and/or authorization of existing endpoints?
  • Does this change introduce new dependencies that need vetting?
  • Does this change require changes to our infrastructure?
  • Does logging contain sensitive data?
  • Does this PR include or remove any sensitive information itself?

If you answered 'yes' to any of the questions above, conduct a detailed Review that addresses at least:

  • What are the potential security threats and mitigations? Please list the STRIDE threats and how they are mitigated
    • Spoofing (faking authenticity)
      • Threat T, which could be achieved by A, is mitigated by M
    • Tampering (influence or sabotage the integrity of information, data, or system)
    • Repudiation (the ability to dispute the origin or originator of an action)
    • Information disclosure (data made available to entities who should not have it)
    • Denial of service (make a resource unavailable)
    • Elevation of Privilege (reduce restrictions that apply or gain privileges one should not have)
  • Have you ensured logging does not contain sensitive data?
  • Have you received any additional approvals needed for this change?

Pull reviewers stats

Stats for the last 30 days:

User Total reviews Time to review Total comments
jeremy-page 24 36m 3
kevinhaube 14 17h 6m 21
carlosfelix2 13 14h 16m 41
TomNUSDS 7 1h 19m 18
clediggins-usds 7 1h 3m 5
bgantick 5 1h 22m 1
brick-green 5 30m 0
ahay-agile6 4 2h 33m 2
mreifman 3 32m 1
oslynn 3 13h 24m 2
jorg3lopez 3 2h 30m 0
MauriceReeves-usds 3 1h 50m 0
sethdarragile6 3 1h 50m 2
TurkeyFried 2 27m 1
cwinters-usds 2 4h 35m 4
JosiahSiegel 1 3m 0
RickHawesUSDS 1 6m 2
snesm 1 10h 44m 0
jh765 1 32m 2

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2022

This pull request introduces 1 alert when merging 05b017c into 64a1601 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@kevinhaube kevinhaube self-assigned this May 31, 2022
@kevinhaube kevinhaube added frontend Work Type label to flag work related to the front-end websites experience Team label to flag issues owned by the Experience Team labels May 31, 2022
@kevinhaube kevinhaube marked this pull request as ready for review May 31, 2022 20:07
# Conflicts:
#	frontend-react/src/components/header/AdminDropdownNav.tsx
@kevinhaube kevinhaube requested review from doug-s-nava and removed request for aurora-a-k-a-lightning May 31, 2022 21:13
@@ -63,6 +67,11 @@
"yarn:show-outdated-packages": "yarn outdated",
"run-build-dir": "yarn build:localdev:csp && yarn global add serve && serve -s build"
},
"jest": {
"transformIgnorePatterns": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Big ugly line of config to get jest and react-markdown to play nice

Copy link
Contributor

Choose a reason for hiding this comment

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

ouch! wonder if there's any way to point to this thread for further explanation remarkjs/react-markdown#635 (comment)

Copy link
Contributor

@doug-s-nava doug-s-nava left a comment

Choose a reason for hiding this comment

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

looks very cool on my first pass! just a few nits here and there. I'm not sure how to test manually, maybe we can run through that tomorrow

@@ -63,6 +67,11 @@
"yarn:show-outdated-packages": "yarn outdated",
"run-build-dir": "yarn build:localdev:csp && yarn global add serve && serve -s build"
},
"jest": {
"transformIgnorePatterns": [
Copy link
Contributor

Choose a reason for hiding this comment

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

ouch! wonder if there's any way to point to this thread for further explanation remarkjs/react-markdown#635 (comment)

@@ -0,0 +1,22 @@
/* Used to instantiate a set of static pages, like BuiltForYouIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks more like a utility class than a component. Maybe it belongs in a utils file somewhere?

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'm on a mission to refactor out the utils files. They tend to become a junk drawer for relatively similar functions a lot of times. That said, I do think it should be either wrapped into a component file or put elsewhere. I think this could easily be merged into the StaticPageFromDirectories code since these are the directories it uses to generate the content. Makes sense to me as a module.

frontend-react/src/components/header/DropdownNav.tsx Outdated Show resolved Hide resolved
frontend-react/src/components/header/DropdownNav.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@doug-s-nava doug-s-nava left a comment

Choose a reason for hiding this comment

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

✅ ✅ ✅ ✅ ✅ ✅ ✅

on the question of where the MarkdownDirectory class should live, I'd still like to see it live somewhere other than Components, since I'd expect everything in there to be renderable as a react component, but that's a personal nit, so this looks good to me to go out as is.

@kevinhaube kevinhaube enabled auto-merge (squash) June 2, 2022 19:45
@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.6% 80.6% Coverage
0.0% 0.0% Duplication

@kevinhaube kevinhaube merged commit d6979c4 into master Jun 2, 2022
@kevinhaube kevinhaube deleted the experience/5383-markdown-static-pages branch June 2, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experience Team label to flag issues owned by the Experience Team frontend Work Type label to flag work related to the front-end websites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web page build - "Built for you: new in Reportstream"
2 participants