Set up Storybook Documentation #113

hreineck · 2021-07-09T20:16:04Z

Made inital setup for writing Storybook docs and
wrote up docs for components related to OSCAL Controls.

Added .stories.js files to the src/stories directory for
each component. These files render the components in the
Storybook stories.

Created a markdown page at src/stories/OSCALDocs.stories.mdx
that can serve as an initial framework for documenting the
Storybook page.
This will include a short description of each control
and the required and optional props for each one.

hreineck · 2021-07-09T20:18:18Z

To view the Storybook page, run:
npm install
npm run storybook
in the root directory.

The .stories.js files in src/stories are responsible for rendering the docs.
They follow the format described in Storybook 's documentation.

kylelaker · 2021-07-09T20:24:19Z

It looks like maybe this is based off an old version of develop which is causing some of these pipeline failures. Could you try doing:

git pull --rebase origin develop
git push --force-with-lease origin EGRC-275

That might pull the commit that fixed these issues to this branch

kylelaker · 2021-07-09T20:36:17Z

hmm it looks like maybe that wasn't the issue... we've caused this issue to re-appear another way. interesting.

tuckerzp · 2021-07-09T21:07:29Z

Does running tests locally work @hreineck?

UPDATE: They don't

hreineck · 2021-07-09T21:13:16Z

Does running tests locally work @hreineck?

No, i get this error that we had encountered before:
Cannot find module '@testing-library/dom' from 'node_modules/@testing-library/user-event/dist/click.js'

tuckerzp · 2021-07-09T21:31:05Z

@hreineck I get the same error. I have tried reinstalling both package-lock files & the node-modules. From what I have seen, the changes to package.json wouldn't cause this and I assume the .story files wouldn't either.

rgauss · 2021-07-09T21:53:04Z

Initial feedback after a quick look here:

Overall, this is a great first start!!
We should look to extend the existing granular test data to be able to support the import of it into stories so we have as little duplication as possible and are testing with data that then surfaces in the docs
For components used across multiple consuming parent components, we should consider the OSCAL 'stack' from bottom up and describe the first story variant as something like Default or ControlOnly (rather than No*) then build with others that add modifications, constraints, etc. Simple cases at the top, increased complexity as we go down the variants (I keep using that term, blame it on the Loki series I guess)
We should think about whether or not we want separate component stories for each component actually exposed in a file.
- As an example, the OSCALControlProse.js file actually exposes two components OSCALReplacedProseWithByComponentParameterValue and OSCALReplacedProseWithParameterLabel because they share significant code, but are for different purposes. Should those two components be separate stories?
Some of the examples use a story title with a prefix of Components/... instead of just the component which Storybook then renders under a Components group in the interface. Perhaps we should investigate what's common and/or which method will lead to a better developer experience.
The Getting Started section should probably focus on how to build/access (npm registry) the library and how to use Storybook, rather than any repeat of the READMEs or components themselves

hreineck · 2021-07-13T15:03:00Z

The Getting Started section should probably focus on how to build/access (npm registry) the library and how to use Storybook, rather than any repeat of the READMEs or components themselves

So should we keep the markdown page listing the required/optional props for each component? Or is that
information covered by the stories themselves?

tuckerzp · 2021-07-13T20:57:29Z

I wonder if linting error is due to not committing the changes to example/package-lock.json?

zclarkEDC · 2021-07-13T21:03:13Z

I wonder if linting error is due to not committing the changes to example/package-lock.json?

This is correct.
If you change the package.json , you need to commit the updated package-lock.json with it.

Bronstrom · 2021-07-16T14:14:20Z

src/stories/OSCALControlImplementation.stories.js

+const exampleImplStatements = {
+  description: "This is the control implementation for the system.",
+  "implemented-requirements": [
+    {
+      uuid: "implemented-requirements-1",
+      "control-id": "control-1",
+      statements: [
+        {
+          "statement-id": "control-1_smt.a",
+          uuid: "f3887a91-9ed3-425c-b305-21e4634a1c34",


Overall, this PR looks pretty good! Although, maybe we could avoid some code duplication in a couple parts that come directly from the test mock data. For example, this code here looks like it could be imported from OSCALControlImplementationImpReq.test.js.

Agreed, that what I was suggesting in my comment:

We should look to extend the existing granular test data to be able to support the import of it into stories so we have as little duplication as possible and are testing with data that then surfaces in the docs.

package.json

Made inital setup for writing Storybook docs and wrote up docs for components related to OSCAL Controls. Added .stories.js files to the src/stories directory for each component. These files render the components in the Storybook stories. Created a markdown page at src/stories/OSCALDocs.stories.mdx that can serve as an initial framework for documenting the Storybook page. This will include a short description of each control and the required and optional props for each one.

Moved all Storybook stories into a "Components" directory. Changed the name and order of the stories of components to list "Default" first, and then variants on the default. Added story for Replaced Prose with By-Component Parameter Value and changed the name of Replaced Prose with Parameter Label accordingly.

Added "@testing-library/dom" to dependecies in package.json in order to fix the broken tests. Seems the storybook init command "npx sb init" was also overwriting and deleting this dependency. Adding it explicity to package.json keeps it from doing that.

Commit changes to example/package-lock.json in order to get the linting checks working.

Some override rules were added to package.json when setting up Storybook. These are removed to clean up the code.

src/stories/OSCALBackMatter.stories.js

src/stories/OSCALControlImplementation.stories.js

src/stories/OSCALControlPart.stories.js

src/stories/OSCALControlProseByComponent.stories.js

src/stories/OSCALMetadata.stories.js

src/stories/OSCALSystemImplementation.stories.js

src/test-data/ModificationsData.js

README.md

Removed the name of the components in front of "With" in the variations to. lign with the rest of the documentation. For example, "BackMatterWithDescription" is now "WithDescription". Metadata.stories were changed also based on this. Consolidated the SystemImplementation variation into the default. Now, the default displays the version and the last modified values. Changed SystemData.js based on consolidating the SystemImplementation.stories.

Fixed some outdated testing for Replaced Prose with Parameter Label. Changes were made to the props that got passed in, but this branch still had test data that did not account for the changes.

Changed the titles of Storybook stories for Control Part and Control Implentation stories to be consistent with the rest of the stories. Changed test data for Modifications to display slightly more verbose information on Storybook. Edited the README to flow better by moving the link to Storybook further up into the Intro section. Deleted the unused file stories/OSCALControlProseByComponent.stories.js

Delete a trailing space in README.md to address the linting failure.

The structure of props passed into Modification components was changed, so the tests and Storybook files were updated to match. Also cleaned up the Modification test data to make these changes in a more efficient way.

rgauss

A few more minor suggestions, but they don't have to hold up approval.

rgauss · 2021-07-23T19:10:37Z

src/test-data/Urls.js

@@ -0,0 +1,13 @@
+/* The following URLs are all of the URLs used in testing and in the stories. */


Is it worth pulling these into their own file?

I think it's a bit cleaner to keep them in their own file, but if we'd rather pull it into something else that's easy enough to do.

Would it make more sense to have the urls that go with specific data go in that data file? The other urls could go with other common data.

For example: backMatterTestUrl goes into BackMatterData.js

Having the urls in their own file will be a nice fix if/when the urls change in the future.

I agree with @zclarkEDC - plus I think the Urls file is justified because some of the Urls are used across a couple different files.

rgauss · 2021-07-23T19:11:21Z

src/test-data/OtherData.js

@@ -0,0 +1,94 @@
+import { backMatterTestData } from "./BackMatterData";
+
+const adds = [


This seems specific to modifications, should it be moved there?

Are you referring to the adds const?

adds is only used in that file and isn't referenced anywhere else, that is why I placed it in OtherData.js (or whatever we rename that file to)

For #124 having the modify, including the adds portion, of a profile is used in places other than just Profile (SSP & Component Definition specifically). Maybe having a ModificationData file makes sense? I am not sure if that is exactly what you mean @rgauss but it could prevent some duplication later.

It is related to modifications, but the only place it's used is the profile test.
I think it'd technically be cleanest to modify the profile test to use the existing
test data in test-data/ModificationsData but that might be more trouble than it's worth
if it requires changing the test in some fundamental way.

I moved this to a more accurately named 'ProfileData.js` - hope that makes this more readable.

src/test-data/OtherData.js

kylelaker

I am still going to keep my "Request changes" until all other approvals are complete and we have made our last push to the branch; this is in regards to on.push.branches[1] and the need to remove that value but needing to not impact the ability to see live deployments while developing.

Rename test-data/OtherData.js to CommonData.js to be a bit more descriptive.

zclarkEDC

All local tests pass

Test Suites: 14 passed, 14 total
Tests:       66 passed, 66 total
Snapshots:   0 total
Time:        17.925 s
Ran all test suites.

Doing npm run storybook successfully builds all modules and opens the Storybook on localhost.

Console error when opening Control Prose -> Parameters Replaced With By-Component Value

This issue is most likely caused in OSCALControlProse.stories.js line 51

ByComp.storyName = "Parameters Replaced With By-Component Value";

Removing this line caused the same error, however as you can see:

It changed the name to what looks like the correct naming scheme.

EDIT: this was fixed with 20d45a5

hreineck · 2021-07-26T17:32:40Z

Console error when opening Control Prose -> Parameters Replaced With By-Component Value
![console error](https://user-images.githubusercontent.com/70411989/127009701-f269c55a-e88e-409e-a3e2-

Seems this is actually caused by this prop.

Not sure how we should fix this though; as far as I can tell OSCALReplacedProseWithByComponentParameterValue calls getStatementByComponent which returns an object. We're trying to then pass that in as the title prop of StyledTooltip by assigning description to be equal to the returned object. This makes it undefined, and passing undefined as a prop gives the error.

What should this title prop actually be? I thought of passing in something like the componentId prop as a workaround, but maybe that's not what's intended.

tuckerzp · 2021-07-26T17:43:47Z

We're trying to then pass that in as the title prop of StyledTooltip by assigning description to be equal to the returned object. This makes it undefined, and passing undefined as a prop gives the error.

When statementByComponent is de-structured, is description always undefined?

What should this title prop actually be? I thought of passing in something like the componentId prop as a workaround, but maybe that's not what's intended.

If description is not undefined, I would think we could still use it? Maybe if it is undefined, using componentId or some other variable would be good. Maybe something like this?

<StyledTooltip title={description ?? props.componentId}>

Removed a console error caused by a `title` prop being undefined under certain circumstances in `OSCALReplacedProseWithByComponentParameterValue` this was done by a quick workaround adding a conditional if the prop is undefined. Also added the `componentId` prop into the ImplReq test data to work with this change and better document the props for Control Prose.

Moved around some test data to make it more readable. Since `otherData` was renamed to `commonData` it makes more sense to move any data that is only used in one place to a more accurately named file. Moved control prose data to ControlsData.js Moved profile test data to new file ProfileData.js

Change the export in ProfileData.js to export default to pass the linting check.

Removed `EGRC-275` from the branches that are watched by the GitHub actions script, so that when this branch is merged it only publishes on changes to develop.

tuckerzp

Everything looks good! Thanks again for all your hard work on this. The storybook looks even cooler than I thought it would.

I was still curious about what everyone thought about Ray's recommendations of:

Putting urls into their relevant components
Creating a modification test data file and moving the adds & modify parameters to that file.

I was just hoping to get a final opinion on what to do for those conversations then I think we should be good to merge.

hreineck requested review from Bronstrom, tuckerzp, rgauss, kkennedy26, kylelaker, zclarkEDC, folksgl and snorouzzadeh July 9, 2021 20:18

hreineck force-pushed the EGRC-275 branch from f299b87 to e93fcbe Compare July 9, 2021 20:27

hreineck force-pushed the EGRC-275 branch from 69a2dd9 to 71dac4e Compare July 13, 2021 17:14

Bronstrom reviewed Jul 16, 2021

View reviewed changes

hreineck linked an issue Jul 16, 2021 that may be closed by this pull request

OSCAL React Components Documentation #109

Closed

kylelaker reviewed Jul 16, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

hreineck added 5 commits July 16, 2021 12:10

Update Package Lock File to Fix Linter Errors

a75226b

Commit changes to example/package-lock.json in order to get the linting checks working.

Remove Unneeded Rules from Package

f041361

Some override rules were added to package.json when setting up Storybook. These are removed to clean up the code.

hreineck force-pushed the EGRC-275 branch from a6de364 to f041361 Compare July 16, 2021 16:10

Add .story.js file for metadata component

54cc259

Merge branch 'develop' into EGRC-275

d56309c

rgauss suggested changes Jul 22, 2021

View reviewed changes

kkennedy26 and others added 3 commits July 23, 2021 11:02

Update Test and Storybook for Control Prose

d5e50eb

Fixed some outdated testing for Replaced Prose with Parameter Label. Changes were made to the props that got passed in, but this branch still had test data that did not account for the changes.

hreineck force-pushed the EGRC-275 branch from 88f4417 to 7893bb9 Compare July 23, 2021 16:44

Address Linting Issue in README

52c9472

Delete a trailing space in README.md to address the linting failure.

hreineck requested a review from rgauss July 23, 2021 17:11

rgauss and others added 2 commits July 23, 2021 13:50

Slight reordering of README

43fd0b9

Update Tests and Storybook Docs to Accomodate Recent Changes

ea00f87

The structure of props passed into Modification components was changed, so the tests and Storybook files were updated to match. Also cleaned up the Modification test data to make these changes in a more efficient way.

kkennedy26 requested a review from kylelaker July 23, 2021 18:58

rgauss reviewed Jul 23, 2021

View reviewed changes

kylelaker suggested changes Jul 23, 2021

View reviewed changes

Rename OtherData to CommonData

fea8a5e

Rename test-data/OtherData.js to CommonData.js to be a bit more descriptive.

zclarkEDC suggested changes Jul 26, 2021

View reviewed changes

zclarkEDC approved these changes Jul 26, 2021

View reviewed changes

hreineck added 3 commits July 26, 2021 23:17

Resolve Linting Issue

4f8b150

Change the export in ProfileData.js to export default to pass the linting check.

Update GitHub Actions Script to Prepare to Merge

0a740b3

Removed `EGRC-275` from the branches that are watched by the GitHub actions script, so that when this branch is merged it only publishes on changes to develop.

tuckerzp reviewed Jul 27, 2021

View reviewed changes

hreineck requested a review from rgauss July 27, 2021 16:37

rgauss approved these changes Jul 27, 2021

View reviewed changes

hreineck requested a review from kylelaker July 27, 2021 17:30

tuckerzp approved these changes Jul 27, 2021

View reviewed changes

kylelaker approved these changes Jul 27, 2021

View reviewed changes

kylelaker merged commit bbdc519 into develop Jul 27, 2021

tuckerzp deleted the EGRC-275 branch July 27, 2021 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up Storybook Documentation #113

Set up Storybook Documentation #113

hreineck commented Jul 9, 2021

hreineck commented Jul 9, 2021 •

edited

kylelaker commented Jul 9, 2021

kylelaker commented Jul 9, 2021

tuckerzp commented Jul 9, 2021 •

edited

hreineck commented Jul 9, 2021

tuckerzp commented Jul 9, 2021 •

edited

rgauss commented Jul 9, 2021

hreineck commented Jul 13, 2021

tuckerzp commented Jul 13, 2021

zclarkEDC commented Jul 13, 2021

Bronstrom Jul 16, 2021

rgauss Jul 21, 2021

rgauss left a comment

rgauss Jul 23, 2021

hreineck Jul 23, 2021

tuckerzp Jul 23, 2021

zclarkEDC Jul 26, 2021

hreineck Jul 27, 2021

rgauss Jul 23, 2021

mikeisen1 Jul 23, 2021

mikeisen1 Jul 23, 2021

tuckerzp Jul 23, 2021 •

edited

hreineck Jul 23, 2021 •

edited

hreineck Jul 27, 2021

kylelaker left a comment

zclarkEDC left a comment •

edited

hreineck commented Jul 26, 2021 •

edited

tuckerzp commented Jul 26, 2021

tuckerzp left a comment •

edited

		@@ -0,0 +1,13 @@
		/* The following URLs are all of the URLs used in testing and in the stories. */

		@@ -0,0 +1,94 @@
		import { backMatterTestData } from "./BackMatterData";

		const adds = [

Set up Storybook Documentation #113

Set up Storybook Documentation #113

Conversation

hreineck commented Jul 9, 2021

hreineck commented Jul 9, 2021 • edited

kylelaker commented Jul 9, 2021

kylelaker commented Jul 9, 2021

tuckerzp commented Jul 9, 2021 • edited

hreineck commented Jul 9, 2021

tuckerzp commented Jul 9, 2021 • edited

rgauss commented Jul 9, 2021

hreineck commented Jul 13, 2021

tuckerzp commented Jul 13, 2021

zclarkEDC commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgauss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuckerzp Jul 23, 2021 • edited

Choose a reason for hiding this comment

hreineck Jul 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelaker left a comment

Choose a reason for hiding this comment

zclarkEDC left a comment • edited

Choose a reason for hiding this comment

hreineck commented Jul 26, 2021 • edited

tuckerzp commented Jul 26, 2021

tuckerzp left a comment • edited

Choose a reason for hiding this comment

hreineck commented Jul 9, 2021 •

edited

tuckerzp commented Jul 9, 2021 •

edited

tuckerzp commented Jul 9, 2021 •

edited

tuckerzp Jul 23, 2021 •

edited

hreineck Jul 23, 2021 •

edited

zclarkEDC left a comment •

edited

hreineck commented Jul 26, 2021 •

edited

tuckerzp left a comment •

edited