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

EGRC-294: Implement Test for Component Definition Control Implementation UI #46

Merged
merged 7 commits into from
May 17, 2021

Conversation

kkennedy26
Copy link
Contributor

Will still need to run npm run lint or remove the prettier/prettier line to resolve linting errors, then run npm test. (Will resolve prettier/prettier end of line errors in another branch)

@kkennedy26 kkennedy26 requested a review from rgauss May 13, 2021 18:36
Copy link
Contributor

@rgauss rgauss left a comment

Choose a reason for hiding this comment

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

A few suggestions.

I believe this is what the build is complaining about:

src/OSCALComponentDefinition.js
--
105 | Line 41:34:  Replace `⏎······props.componentDefinition.components⏎····).map(` with `props.componentDefinition.components).map(⏎······`  prettier/prettier
106 | Line 44:1:   Insert `··`                                                                                                                prettier/prettier
107 | Line 45:1:   Insert `··`                                                                                                                prettier/prettier
108 | Line 46:9:   Insert `··`                                                                                                                prettier/prettier
109 | Line 47:1:   Insert `··`                                                                                                                prettier/prettier
110 | Line 48:7:   Insert `··`                                                                                                                prettier/prettier
111 | Line 49:5:   Replace `)` with `··)⏎····`                                                                                                prettier/prettier
112 |  
113 | src/OSCALSystemCharacteristics.js
114 | Line 222:46:  Replace `⏎································"information-type-ids"` with `"information-type-ids"].map(`  prettier/prettier
115 | Line 224:31:  Replace `].map(` with `··`                                                                             prettier/prettier
116 | Line 225:1:   Replace `································` with `··································`                   prettier/prettier
117 | Line 226:1:   Insert `··`                                                                                            prettier/prettier
118 | Line 227:35:  Insert `··`                                                                                            prettier/prettier
119 | Line 228:1:   Replace `··································` with `····································`               prettier/prettier
120 | Line 229:1:   Insert `··`                                                                                            prettier/prettier
121 | Line 230:35:  Insert `··`                                                                                            prettier/prettier
122 | Line 231:1:   Replace `··································` with `····································`               prettier/prettier
123 | Line 232:1:   Insert `··`                                                                                            prettier/prettier
124 | Line 233:31:  Replace `)` with `··)⏎······························`                                                  prettier/prettier

@@ -57,6 +57,7 @@ export default function OSCALComponentDefinition(props) {
<OSCALComponentDefinitionComponent
component={component}
parties={props.componentDefinition.metadata.parties}
key={key}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is actually needed, i.e. OSCALComponentDefinitionComponent never calls props.key.

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 added this prop because it was giving me a console error since key from the map function was never being assigned. But yes, OSCALComponentDefinitionComponent never calls props.key, instead it calls props.component.uuid.

@@ -38,6 +38,8 @@ const componentDefinitionTestData = {
},
};

export default { componentDefinitionTestData };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be needed.

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 added this because this was part of eslint recommendations of how to export a function. https://eslint.org/docs/rules/no-restricted-exports

});
}

export { getByTextIncludingChildern as default };
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just add export to the function definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above. https://eslint.org/docs/rules/no-restricted-exports..... I could be wrong though.

@kkennedy26
Copy link
Contributor Author

@rgauss, the linting errors with SystemCharacteristics.js, should I resolve them in this branch so it can pass the check? I never made any changes to that file. It is not related to this branch.

@kkennedy26 kkennedy26 merged commit dfd5b67 into develop May 17, 2021
@rgauss rgauss deleted the EGRC-294 branch June 23, 2021 13:52
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.

None yet

2 participants