UX Change; Move Modification icon to the side of control title #52

hreineck · 2021-06-10T20:36:40Z

Changed the react components in OSCALControl.js to render the modification component (OSCALControlModification.js)
instead of it being rendered at the top of OSCALControlPart.js
This satisfies the UX change specified in EGRC-306.

Changed the react components in OSCALControl.js to render the modification component (OSCALControlModification.js) instead of it being rendered at the top of OSCALControlPart.js This satisfies the UX change specified in EGRC-306.

rgauss

We need resolve conflicts here first, and check with @kkennedy26 on the check for whether or not to display the modification as mentioned in comments.

rgauss · 2021-06-11T11:52:05Z

src/OSCALControl.js

@@ -58,6 +59,26 @@ export default function OSCALControl(props) {
  }
  const classes = useStyles(props);

+  let modificationDisplay;
+  if (props.modifications) {
+    // Finds the control-id within alters and matches it with a resolved control


I think all of this has been moved into the OSCALControlModification component and does not need to be duplicated here.

rgauss · 2021-06-11T11:53:13Z

src/OSCALControlModification.js

      <Tooltip title="Modifications">
        <Badge
          anchorOrigin={{
            vertical: "top",
-            horizontal: "right",
+            horizontal: "left",


I think this should stay to the right.

…EGRC-306

OSCALControlPart.js renders the modification icon for each control part. Before, this rendered the icon under the title, which was undesirable as referenced in EGRC-306. OSCALControl.js now renders the icon next to the title, and passes in null as the modifications argument to its top-level control part so that the icon is not rendered in OSCALControlPart.js Also, in OSCALControlModification.js the <div> around the icon was removed so that it would render next to the title.

The const topPart variable in OSCALControl.js was out of place and was causing tests to fail. The line was deleted.

rgauss · 2021-06-11T20:16:26Z

src/OSCALControl.js

+    modificationDisplay = (
+      <OSCALControlModification
+        modifications={props.modifications}
+        controlPartId={props.control.parts[0].id}


I don't think we want to specify a controlPartId at all here.

rgauss · 2021-06-11T20:17:29Z

src/OSCALControl.js

              control={props.control}
+              modifications={index !== 0 && props.modifications}


What is this change trying to do?

rgauss · 2021-06-11T20:18:12Z

src/OSCALControlModification.js

@@ -145,7 +145,7 @@ export default function OSCALControlModification(props) {
  // Display if alter or controlPartObject is true
  if (alter || controlPartObject) {
    modificationsDisplay = (
-      <div>
+      <>


If we don't need a div can we remove the empty <> element?

Believe that's the only way to have a component without a div around it

@hreineck I was wondering what removing the <div> does here? I was just looking over it for suggestions and genuinely didn't know.

div adds a divider between the icon and the text it's supposed to be next to, so I removed it to render in line with the text

Would a <span> accomplish the same thing?

I tested it locally, and from what I can see it works.

True. Span would be another way of doing it inline. Maybe that would be more descriptive than the empty <>

rgauss · 2021-06-11T20:18:35Z

src/OSCALControlModification.js

    );
  } else {
    modificationsDisplay = null;
  }

-  return <div>{modificationsDisplay}</div>;
+  return <>{modificationsDisplay}</>;


Why the empty <> element here as well?

src/OSCALControlPart.js

rgauss · 2021-06-11T20:20:53Z

src/OSCALControlPart.js

      if (!parameter) {
        return;
      }
      // TODO parse select parameters
      return `< ${parameter.label} >`;
    }

-    replacedProse = props.prose.replace(/\{\{ insert: param, ([0-9a-zA-B-_.]*) \}\}/g, getParameterLabel);
+    replacedProse = props.prose.replace(


Were these types of changes from linter complaints?

Moved the call to OSCALControlModification in OSCALControlParts into the typography component of the replaced prose so that it renders next to the control part. Fixed some issues with rendering the top level control modifications: OSCALControl now correctly passes in its own control id when displaying its modifications. Also some minor cleanup in OSCALControlModification.js These changes modify the UX to satisfy EGRC-306.

rgauss · 2021-06-17T16:31:38Z

@hreineck, there may have been some overlapping changes with what was going on in #53.

Can you sync merge EGRC-261 into this branch and make sure everything still looks right?

tuckerzp

I believe places with empty <> can be replaced with a <span>. <div> is a block level element while <span> is an inline element.

rgauss · 2021-06-17T16:49:11Z

@hreineck and @tuckerzp, I did a sync merge.

The top-level modifications are repeating for each control part as well.

I think the call to OSCALControlModification now needs to send in controlId instead of control, and we may not need any of the changes in OSCALControlPart?

rgauss · 2021-06-17T16:49:36Z

src/OSCALControl.js

+      <OSCALControlModification
+        modifications={props.modifications}
+        controlPartId={props.control.id}
+        control={props.control}


I think this now needs to be controlId?

src/OSCALControlPart.js

Changed OSCALModification to distinguish between top-level modifications and control parts. Some profiles do not include a "by-id" field on top-level modifications; added a check to get around this so that only the control title renders the top-level icon.

tuckerzp · 2021-06-17T18:40:02Z

src/OSCALControl.js

@@ -78,8 +90,8 @@ export default function OSCALControl(props) {
              parameters={props.control.params}
              implReqStatements={props.implReqStatements}
              componentId={props.componentId}
-              modifications={props.modifications}
              control={props.control}


This can be changed to use the id rather than the entire control now. That would also make check in OSCALControlPart unneeded.

tuckerzp · 2021-06-17T18:41:32Z

src/OSCALControlModification.js

@@ -122,6 +122,7 @@ export default function OSCALControlModification(props) {
    if (alter.adds) {
      [addsDisplay, len] = getModifications(
        props.controlPartId,
+        props.controlPartId === props.controlId,


Would it be cleaner to just pass the controlId? I feel like that would give us more relevant information later on, for things like debugging, than just a bool. Either way would probably work

You would just push the check to isRelevantId where we are already doing logic checking similar to that

Yes, this is what I had done as well, see above comment on isRelevantId.

tuckerzp · 2021-06-17T18:42:07Z

src/OSCALControlModification.js

@@ -137,7 +138,7 @@ export default function OSCALControlModification(props) {
  // Display if altered is true
  if (modLength) {
    modificationsDisplay = (
-      <div>
+      <span>


tuckerzp · 2021-06-17T18:56:16Z

src/OSCALControlModification.js

@@ -61,11 +61,11 @@ const getAlterAddsOrRemovesDisplay = (addsElements, addsLabel) => {
 * @param {String} field Field of Add/Remove element to check
 * @returns true if element matches controlID
 */
-const isRelevantId = (controlPartId, element, field) =>
+const isRelevantId = (controlPartId, element, field, isTopLevel) =>


Changing the documentation to show change in parameters might be good to do now rather than figure it out in documentation issue. Same for getModifications

Let's instead send in the controlId (maybe as the first argument) and a null controlPartId when calling from a top-level control, and yes, let's update the docs.

rgauss · 2021-06-17T19:05:02Z

src/OSCALControlModification.js

@@ -61,11 +61,11 @@ const getAlterAddsOrRemovesDisplay = (addsElements, addsLabel) => {
 * @param {String} field Field of Add/Remove element to check
 * @returns true if element matches controlID
 */
-const isRelevantId = (controlPartId, element, field) =>
+const isRelevantId = (controlPartId, element, field, isTopLevel) =>


Let's instead send in the controlId (maybe as the first argument) and a null controlPartId when calling from a top-level control, and yes, let's update the docs.

rgauss · 2021-06-17T19:06:30Z

src/OSCALControlModification.js

@@ -122,6 +122,7 @@ export default function OSCALControlModification(props) {
    if (alter.adds) {
      [addsDisplay, len] = getModifications(
        props.controlPartId,
+        props.controlPartId === props.controlId,


Yes, this is what I had done as well, see above comment on isRelevantId.

Removed the redundant props passed into OSCALModification from OSCALControl Changed the structure of the isRelevantId function to be more self-describing Updated function docs

My concerns have been addressed

kylelaker · 2021-06-18T19:51:42Z

src/OSCALControl.js

@@ -69,7 +80,7 @@ export default function OSCALControl(props) {
          className={classes.OSCALControlChildLevelTitle}
        >
          <span className={classes.OSCALControlId}>{props.control.id}</span>{" "}
-          {props.control.title}
+          {props.control.title} {modificationDisplay}


What happens if (!props.modifications) above and therefore and modificationDisplay == undefined? Not reviewing, just asking a React question. Does it just not render this bit?

It should just not render anything

kylelaker · 2021-06-18T20:55:01Z

src/OSCALControlModification.js

  // TODO: Differences in how NIST and FedRAMP implement adds makes
  // this check needed. See if we can clean this up at some point.
-  // Assumes the difference is that NIST does not have a "control-id" field
-  !element[field] || element[field] === controlPartId;
+  // Assumes the difference is that NIST does not have a "by-id" field


This doesn't really make that assumption does it? It allows callers to make that assumption? Basically, the old comment was incorrect as well right? Can we remove it?

I think more accurately there should be a comment where we call this check with"by-id" as an argument

kylelaker · 2021-06-18T20:59:10Z

src/OSCALControlModification.js

    );
  } else {
    modificationsDisplay = null;
  }

-  return <div>{modificationsDisplay}</div>;
+  return modificationsDisplay;


With the removal of the <div> you, can now just return directly in the if above without having to keep this extra mutable var.

if (!modLength) { return null; } return ( <span>...

kylelaker · 2021-06-18T21:03:07Z

src/OSCALControlModification.js

+  (!element[field] && controlPartId === controlId) ||
+  element[field] === controlPartId;


Now I feel guilty for having helped shorten this earlier as the number of variables and conditions begins to grow. I am losing understanding of the implementation here. Could we do something like:

const nistCheck = (controlPartId, controlId, element, field) => ... const fedrampCheck = (controlPartId, controlId, element, field) => ... return nistCheck(...) || fedrampCheck(...)

or even just some well-written if statements with early returns that are well commented.

It sounds like each of them defines things in a certain way. But we talk a lot about how we have to do this check because they're different. And then we lose all sight of how they differ in the implementation. But we also want to do something extensible because if NIST and FedRAMP differ here, then is it possible for Agency C to come along and do it differently as well?

These variations aren't specific to NIST or FedRAMP, just different ways to represent control modifications in the OSCAL spec, so I don't think we want to start naming methods around their implementations.

I'm fine with splitting things out into separate conditionals for better readability if need be though.

So are these the only ways to express the control modifications? Or would there be others? If these two are the only ways we're going to have to do it, then yeah, separate conditionals explaining would make sense. But the surrounding comments right now are very name-focused (NIST & FedRAMP) and not data structure focused (how it relates to the spec) so that was the idea behind using the names.

But I think the fact there's ambiguity there at all with the comments and complexity of the expression is a good indication we should try to clean it up at some point. @hreineck if there's a good way to clean this up during this PR that doesn't creep the scope of what you're actually working on, that'd be cool to see but I want to make sure I am not asking for something beyond what you're already doing.

I would like to see Kyle's reccomendations considered

Edited comments in OSCALControlModification.js to more accurately explain the isRelevantID check, which is needed for different ways to represent control modifications in OSCAL. Cleaned up the modification React component; removed an uncessesary variable.

kylelaker · 2021-06-21T15:40:08Z

Thanks @hreineck for the changes! Good stuff!

hreineck requested review from rgauss, zclarkEDC and kkennedy26 June 10, 2021 20:36

rgauss suggested changes Jun 11, 2021

View reviewed changes

hreineck added 2 commits June 11, 2021 15:18

Merge branch 'EGRC-261' of github.com:EasyDynamics/oscal-viewer into …

146a995

…EGRC-306

hreineck requested a review from rgauss June 11, 2021 20:03

Cleaned up OSCALControl.js

ead0bcf

The const topPart variable in OSCALControl.js was out of place and was causing tests to fail. The line was deleted.

rgauss suggested changes Jun 11, 2021

View reviewed changes

hreineck requested review from rgauss, folksgl and kylelaker June 17, 2021 14:56

tuckerzp suggested changes Jun 17, 2021

View reviewed changes

Merge branch 'EGRC-261' into EGRC-306

b6bce4b

rgauss suggested changes Jun 17, 2021

View reviewed changes

hreineck requested review from rgauss and tuckerzp June 17, 2021 18:29

tuckerzp previously requested changes Jun 17, 2021

View reviewed changes

tuckerzp reviewed Jun 17, 2021

View reviewed changes

rgauss suggested changes Jun 17, 2021

View reviewed changes

Clean up for readability

70a3d63

Removed the redundant props passed into OSCALModification from OSCALControl Changed the structure of the isRelevantId function to be more self-describing Updated function docs

hreineck requested review from tuckerzp and rgauss June 17, 2021 19:39

rgauss approved these changes Jun 17, 2021

View reviewed changes

tuckerzp previously approved these changes Jun 18, 2021

View reviewed changes

kylelaker reviewed Jun 18, 2021

View reviewed changes

kkennedy26 approved these changes Jun 21, 2021

View reviewed changes

Clean up and clarify comments

11e6ca9

Edited comments in OSCALControlModification.js to more accurately explain the isRelevantID check, which is needed for different ways to represent control modifications in OSCAL. Cleaned up the modification React component; removed an uncessesary variable.

hreineck requested a review from kylelaker June 21, 2021 15:31

kylelaker approved these changes Jun 21, 2021

View reviewed changes

hreineck merged commit d883a6d into EGRC-261 Jun 21, 2021

kylelaker deleted the EGRC-306 branch June 21, 2021 16:05

		control={props.control}
		modifications={index !== 0 && props.modifications}

		(!element[field] && controlPartId === controlId) \|\|
		element[field] === controlPartId;

UX Change; Move Modification icon to the side of control title #52

UX Change; Move Modification icon to the side of control title #52

Conversation

hreineck commented Jun 10, 2021

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

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

rgauss commented Jun 17, 2021

tuckerzp left a comment

Choose a reason for hiding this comment

rgauss commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuckerzp Jun 17, 2021 • 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

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

Choose a reason for hiding this comment

kylelaker commented Jun 21, 2021

tuckerzp Jun 17, 2021 •

edited