fix(catalog): Withdrawn "top" controls display correctly #720

Bronstrom · 2023-03-07T20:09:37Z

This fixes the "state" of withdrawn "top-level"
controls. Instead of being a collapsible item
that shows a blank box when selected, the
control no longer opens and closes, as
indicated by no presence of an arrow
appearing to the far right of the item. The
item text is also styled to appear a light-gray
and has a strikethrough, like the "sub-level"
controls.

Resolves #635

This fixes the "state" of withdrawn "top-level" controls. Instead of being a collapsible item that shows a blank box when selected, the control no longer opens and closes, as indicated by no arrow appearing to the far right of the item. The item text is also styled to appear a light-gray and has a strikethrough, like the "sub-level" controls.

tuckerzp

This looks like a pretty good start so far! I had a small comment and a thought.

I will also post this for other's reviewing

tuckerzp · 2023-03-07T20:57:24Z

src/components/OSCALCatalogGroup.js

+  const withdrawn = isWithdrawn(control);
+  const itemText = `${control.id.toUpperCase()} ${control.title}`;
+  // Make collapsible item when not a withdrawn control
+  return !withdrawn ? (


This is kind of just a general thought, but I wish that we did not use these super long ternary operators everywhere. They are very gross. Not sure if there is much you can do to avoid it here however.

src/components/OSCALCatalogGroup.js

kylelaker · 2023-03-07T22:17:23Z

src/components/oscal-utils/OSCALCatalogUtils.js

+export default function isWithdrawn(control) {
+  return control?.props?.find(
+    (prop) => prop.name === "status" && prop.value === "withdrawn"
+  );
+}


Let's actually do namespace validation. Some of this code probably belongs in a different file but I am thinking of an API a bit like this (ignore the types if the resulting file has to be JS):

/** * When a namespace is not specified for a property, the assumption is that the * namespace is default namespace. */ const NIST_DEFAULT_NAMESPACE = "https://csrc.nist.gov/ns/oscal"; export interface PropFilter { /** * The namespace of the property. * * @default https://csrc.nist.gov/ns/oscal */ ns?: string; /** * The name of the prop to find. */ name: string; /** * The acceptable value of the property. * * Either this property or {@link filter} (but not both) must be specified. */ value?: string; /** * A function to validate the property value. * * Either this property or {@link value} (but not both) must be specified. */ filter?: (value: string) => boolean; } /** * Return the given namespace, or if undefined, the default namespace. */ export function namespaceOf(ns: string | undefined): string { return ns ?? NIST_DEFAULT_NAMESPACE; } /** * Returns the first matching property in the given list. * * @param props the list of properties to check * @param filter the attributes to match against */ export function matchingProp(props: Prop[], filter: PropFilter): Prop | undefined { const ns = namespaceof(filter.ns); if ((!filter.value && !filter.filter) || (filter.value && filter.filter)) { throw new Error("Exactly one of filter or value must be specified"); } return props.find((prop) => namespaceOf(prop.ns) === namespaceOf(filter.namespace) && prop.name === filter.name && (!filter.value || prop.value === filter.value) && (!filter.filter || filter.filter(value)) ); } export function isWithdrawn(control: Control): boolean { // By default controls are *not* withdrawn if (!control.props) { return false; } return !!matchingProp(control.props, { name: "status", value: "withdrawn }); }

Besides isWithdrawn, most of this probably deserves to exist more in an oscal-utils/OSCALPropUtils.js.

(Eventually, we may want to support class in that filter interface but I do not want to deal with that today)

I've gone ahead and added namespace validation for JS - types have been removed as well as the interface for now. There were a couple other variable corrections I had to make, but I was able to figure them out.

I've separated isWithdrawn from the rest of the functions oscal-utils/OSCALPropUtils.js but followed the same logic as mentioned. I've just kept the isWithdrawn function in the Catalog Utils class I created. Let me know if something else works better here.

kylelaker · 2023-03-07T22:37:40Z

I am also worried that the font color with the background could be an accessibility issue... I wonder if there's something we can do there (or maybe we wait and try to work on a high-contrast theme later?)

Bronstrom · 2023-03-08T22:38:54Z

I am also worried that the font color with the background could be an accessibility issue... I wonder if there's something we can do there (or maybe we wait and try to work on a high-contrast theme later?)

Where the other withdrawn controls appear (sub-level controls), a theme.palette color wasn't declared there and just a hex code for a gray color. I ended up just matching the top-level controls to this. It would probably be best we have these align to the theme.palette. In the past we have just declared something like theme.palette.grey[50] to produce darker/lighter shades of palette colors, which could work in this case. I believe "50" aligns to opacity.

Co-Authored-By: Kyle Laker <klaker@easydynamics.com>

Bronstrom · 2023-03-09T16:57:45Z

In the past we have just declared something like theme.palette.grey[50] to produce darker/lighter shades of palette colors, which could work in this case. I believe "50" aligns to opacity.

This actually refers to shade in material UI, which has a range of 50 (lightest) to 900 (darkest). We consistently use shade 50 and 400. 400 seems to strike a better balance for now, not too light for the background and not too dark to separate withdrawn controls not withdrawn controls.

kylelaker · 2023-03-09T18:55:53Z

src/components/OSCALCatalogGroup.js

@@ -24,6 +25,11 @@ const StyledListItemPaper = styled(Paper)`
  border-radius: 0.5em;
 `;

+const StyledListItemText = styled(ListItemText)(({ theme }) => ({


Suggested change

const StyledListItemText = styled(ListItemText)(({ theme }) => ({

const WithdrawnListItemText = styled(ListItemText)(({ theme }) => ({

Let's specify how it's styled and what it actually represents. Imagine some day we want to signify "important" controls (or some other made up descriptor) somehow -- those would be styled too but perhaps in a different way. So StyledListItemText doesn't really get the point across in the same way.

I know we've made this mistake elsewhere, but let's do better here.

kylelaker · 2023-03-09T18:58:51Z

src/components/oscal-utils/OSCALCatalogUtils.js

+ * TODO: This is probably 800-53 specific and it should be made more
+ * generic to allow the library to work with other frameworks.
+ * https://github.com/EasyDynamics/oscal-react-library/issues/504


This is now handled. props[@name="status" and @value="withdrawn"] is how to do this per the spec and we now handle the namespace field correctly for this.

Oh true. I guess we could close out #504 once this PR is merged?

src/components/oscal-utils/OSCALPropUtils.js

kylelaker · 2023-03-09T19:01:21Z

src/components/oscal-utils/OSCALPropUtils.js

+/**
+ * Return the given namespace, or if undefined, the default namespace.
+ */
+export function namespaceOf(ns) {
+  return ns ?? NIST_DEFAULT_NAMESPACE;
+}


Suggested change

/**

* Return the given namespace, or if undefined, the default namespace.

*/

export function namespaceOf(ns) {

return ns ?? NIST_DEFAULT_NAMESPACE;

}

/**

* Return the given namespace, or if undefined, the default namespace.

*

* @param {string | undefined} ns the namespace

* @returns {string} the given namespace or the the default namespace

*/

export function namespaceOf(ns) {

return ns ?? NIST_DEFAULT_NAMESPACE;

}

kylelaker · 2023-03-09T19:09:01Z

src/components/oscal-utils/OSCALPropUtils.js

+/**
+ * Returns the first matching property in the given list.
+ *
+ * @param props the list of properties to check
+ * @param filter the attributes to match against
+ */
+export default function matchingProp(props, filter) {


Suggested change

/**

* Returns the first matching property in the given list.

*

* @param props the list of properties to check

* @param filter the attributes to match against

*/

export default function matchingProp(props, filter) {

/**

* @callback PropertyFilterValueMatcher

* @param {string} value - the value to check against

* @returns {boolean} whether the given property matches

*/

/**

* A filter to apply to properties to find a matching property.

*

* This allows matching based on the namespace (defaulting to the default namespace),

* the name, and the value. Exactly one of `value` or `filter` must be supplied.

*

* @typedef PropertyFilter

* @type {object}

* @property {string|undefined} ns - the namespace to check in

* @property {string| name - the name of the field to match

* @property {string} value - the value of the field to match

* @property {PropertyFilterValueMatcher} filter - a function to match the value against

*/

/**

* Returns the first matching property in the given list.

*

* @param props {Array.<Object>} the list of properties to check

* @param filter {PropertyFilter} the attributes to match against

*/

export default function matchingProp(props, filter) {

I think we can represent the types from before as JSDoc types if we want to... I don't know whether this is useful enough or not.

Would this info make more sense to add once we add TypeScript? . . . or maybe this could be helpful to who implements TS here?

Fair yeah -- maybe we just add it then.

Reviewing over it again, this documentation is valuable even now with JS and TS support is just around the corner.

Co-Authored-By: Kyle Laker <klaker@easydynamics.com>

kylelaker

Good stuff!

tuckerzp · 2023-03-09T19:49:07Z

src/components/oscal-utils/OSCALCatalogUtils.js

+ * Determines if a control is withdrawn.
+ *
+ * @param {object} control
+ * @returns a boolean describing whether the control is withdrawn


Can we also update this @returns?

Suggested change

* @returns a boolean describing whether the control is withdrawn

* @returns {boolean} true if control is withdrawn

tuckerzp

This looks great! Thanks for your work on this @Bronstrom

tuckerzp reviewed Mar 7, 2023

View reviewed changes

kylelaker suggested changes Mar 7, 2023

View reviewed changes

Bronstrom and others added 2 commits March 9, 2023 11:24

feat(utils): Add JS namespace validation

5d0ae7d

Co-Authored-By: Kyle Laker <klaker@easydynamics.com>

feat(catalog): Use theme palette for font color

3955f68

Bronstrom added 2 commits March 9, 2023 11:59

feat(catalog): Fix linter errors

7680b09

feat(catalog): Fix linter errors 2

6289f5b

kylelaker reviewed Mar 9, 2023

View reviewed changes

src/components/oscal-utils/OSCALPropUtils.js Outdated Show resolved Hide resolved

feat: Adjust variables & remove issue comment

0a96576

kylelaker reviewed Mar 9, 2023

View reviewed changes

Bronstrom marked this pull request as ready for review March 9, 2023 19:10

feat(catalog): Enhance OSCALPropUtils JSDoc

a419eca

Co-Authored-By: Kyle Laker <klaker@easydynamics.com>

kylelaker approved these changes Mar 9, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team March 9, 2023 19:32

feat(catalog): Add missing "s" to returns comment

b1db57c

tuckerzp reviewed Mar 9, 2023

View reviewed changes

easy-dynamics-oscal-automation bot requested a review from a team March 9, 2023 19:49

feat(utils): Add to OSCALCatalogUtils JSDoc

105fba0

tuckerzp approved these changes Mar 9, 2023

View reviewed changes

kylelaker merged commit 3e40c31 into develop Mar 9, 2023

kylelaker deleted the brad/withdrawn-top-level-controls-not-identifiable branch March 9, 2023 21:09

kylelaker mentioned this pull request May 3, 2023

Unable to expand withdrawn controls #743

Closed

kylelaker added the bug Something isn't working label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(catalog): Withdrawn "top" controls display correctly #720

fix(catalog): Withdrawn "top" controls display correctly #720

Bronstrom commented Mar 7, 2023 •

edited

tuckerzp left a comment •

edited

tuckerzp Mar 7, 2023

kylelaker Mar 7, 2023 •

edited

kylelaker Mar 7, 2023

Bronstrom Mar 9, 2023

Bronstrom Mar 9, 2023

kylelaker commented Mar 7, 2023

Bronstrom commented Mar 8, 2023

Bronstrom commented Mar 9, 2023

kylelaker Mar 9, 2023

kylelaker Mar 9, 2023

Bronstrom Mar 9, 2023 •

edited

kylelaker Mar 9, 2023

kylelaker Mar 9, 2023

Bronstrom Mar 9, 2023

kylelaker Mar 9, 2023

Bronstrom Mar 9, 2023

kylelaker left a comment

tuckerzp Mar 9, 2023

tuckerzp left a comment

	const StyledListItemText = styled(ListItemText)(({ theme }) => ({
	const WithdrawnListItemText = styled(ListItemText)(({ theme }) => ({

-/**
- * Returns the first matching property in the given list.
- *
- * @param props the list of properties to check
- * @param filter the attributes to match against
- */
-export default function matchingProp(props, filter) {
+/**
+ * @callback PropertyFilterValueMatcher
+ * @param {string} value - the value to check against
+ * @returns {boolean} whether the given property matches
+ */
+/**
+ * A filter to apply to properties to find a matching property.
+ *
+ * This allows matching based on the namespace (defaulting to the default namespace),
+ * the name, and the value. Exactly one of `value` or `filter` must be supplied.
+ *
+ * @typedef PropertyFilter
+ * @type {object}
+ * @property {string|undefined} ns - the namespace to check in
+ * @property {string| name - the name of the field to match
+ * @property {string} value - the value of the field to match
+ * @property {PropertyFilterValueMatcher} filter - a function to match the value against
+ */
+/**
+ * Returns the first matching property in the given list.
+ *
+ * @param props {Array.<Object>} the list of properties to check
+ * @param filter {PropertyFilter} the attributes to match against
+ */
+export default function matchingProp(props, filter) {

	* @returns a boolean describing whether the control is withdrawn
	* @returns {boolean} true if control is withdrawn

fix(catalog): Withdrawn "top" controls display correctly #720

fix(catalog): Withdrawn "top" controls display correctly #720

Conversation

Bronstrom commented Mar 7, 2023 • edited

tuckerzp left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelaker Mar 7, 2023 • edited

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

kylelaker commented Mar 7, 2023

Bronstrom commented Mar 8, 2023

Bronstrom commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bronstrom Mar 9, 2023 • edited

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

kylelaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuckerzp left a comment

Choose a reason for hiding this comment

Bronstrom commented Mar 7, 2023 •

edited

tuckerzp left a comment •

edited

kylelaker Mar 7, 2023 •

edited

Bronstrom Mar 9, 2023 •

edited