Add OSCALJsonEditor Component #265

pkothare · 2022-02-21T20:58:04Z

Uses react-split to create a vertically split element into two panes. This only manifests in REST mode and starts off with 50/50 split for the two panes. The end user is then able to adjust the width of the pane up to a certain limit.

The left pane wraps the OSCALJsonEditor component which consists of a configured instance of the @monaco-editor component. The editor is does take up 100% of the vertical height of the HTML page, instead, it adapts to screen height and is positioned relative to the scrollport based on sticky positioning.
The right pane wraps the viewer, which based on the route is one of the specific OSCALLoader components.

TO DOs:

Migrate static CSS to makeStyles used by MUI
Use fetch to complete PUT request
Write tests
Fix overflow issues with OSCAL components. MUI 4 has significant issues element overflows when generic spacing is used.

References

Added conditional checks for `isRestMode` and added the react-split control so that when `isRestMode` is `true` the `OSCALLoader` component will render a split pane.

The `maxWidth` value of `xl` was added which overrides the default of large. This was done so that the JSON editor can be accomodated in the application.

The split pane defaults were updated so that the JSON Editor and viewer share a 50/50 split. The `minSize` value was also updated to 500 as a sensible default, because collapsing a pane to 0 can cause issues.

The updates improve the look and feel based on the margins and grid layout. The editor was also made sticky so that it follows veritical scrolling.

Removed overflow attributes, since they cause conflicts with sticky HTML elements.

The split pane defaults were updated so that the JSON Editor and viewer share a 50/50 split. The `minSize` value was also updated to 500 as a sensible default, because collapsing a pane to 0 can cause issues.

…eact-library into feature/json-editor

kylelaker · 2022-02-25T19:46:28Z

Seems like the content uses ~100% of the display width in non-REST mode now. Is that an intentional change?

Other small nitpicks on UI stuff:

The dropdown for the document and the toggle for REST mode should be aligned (REST mode toggle is currently higher)
The top bar goes away when scrolling -- do we want to keep it? maybe not?
Save/Discard buttons require scrolling to see (on my monitor)

pkothare · 2022-02-25T20:53:53Z

Seems like the content uses ~100% of the display width in non-REST mode now. Is that an intentional change?

Intentional, based on the screenshot that @rgauss shared with us on the potential design of including an editor. If we change the display width between rest/non-rest mode, the layout changes pretty dramatically which leads to some cognitive load. Nonetheless, this has been factored by a very small change to maxWidth. So if we decide to go smaller we can. 🤞, all other components are dynamically sized.

The dropdown for the document and the toggle for REST mode should be aligned (REST mode toggle is currently higher)

Worked this in already, the changes will be up soon.

The top bar goes away when scrolling -- do we want to keep it? maybe not?

I tried not to modify existing layout/style decisions. Prior to this PR, the header was not fixed, so no change there. However changing it to be fixed isn't that hard.

Save/Discard buttons require scrolling to see (on my monitor).

This was intentional, so that when you do scroll past a certain point (i.e. when the top nav and selector are past the scroll port), the editor (including the buttons) fills up the viewport vertical space. Eventually the OSCALLoaderForm will move to a left hand drawer which will pull the editor and the buttons up, and no scroll will be required. We can either reduce the height now, or it can be part of the update when we develop the left hand drawer. It's mostly a matter of testing with different Editor Vertical Height values

kylelaker

This looks good. There are things it might be sorta nice to fix in future tickets but that I don't think should slow down our progress here. A few of them are:

base64 encoding should be handled as part of the build process, we should avoid directly writing b64 strings directly into the application (we do have Webpack here which may help)
it would be cool if the browser find functionality worked within the JSON editor. it has it's own (better) find functionality but you have to click within it and if you don't the browser doesn't see any of the Editor content
TABS?! 😆

pkothare · 2022-02-27T06:13:42Z

base64 encoding should be handled as part of the build process, we should avoid directly writing b64 strings directly into the application (we do have Webpack here which may help)

Webpack should help here, after doing some exploratory testing, I feel like this should be improved as part of another PR, since there are some decisions that need to be made around which one of the following paths we should use to bundle/package the library:

microbundle-crl
webpack
webpack + microbundle-crl

Currently we are using the microbundle-crl to create the bundle. If in the future we decide to use webpack, it will help optimize packaging; and IMO my least favorite option is to use webpack + microbundle-crl because it will introduce too much complexity in packaging.

it would be cool if the browser find functionality worked within the JSON editor. it has it's own (better) find functionality but you have to click within it and if you don't the browser doesn't see any of the Editor content

Confirmed that I can replicate this, the browser find only matches text in the current viewport, this is because the editor is maintains the rest of the text in virtual DOM. Not sure how we get around it, unless we render all the lines. However, the OSCAL JSON files can sometimes be 70K+ lines, which when rendered might lead to a poor experience. Maybe there is a better way that I just haven't discovered yet 🤷

TABS?! 😆

Note for future improvement: Tab Trapping.

src/components/OSCALLoader.js

src/components/OSCALLoaderForm.js

src/components/OSCALLoader.js

src/components/OSCALJsonEditor.js

rgauss · 2022-02-27T23:40:02Z

src/components/OSCALLoader.js

+    result = isRestMode ? (
+      <Split className={classes.split} minSize={500} sizes={[50, 50]}>
+        <Box>
+          <OSCALJsonEditor value={oscalData} onSave={handleRestPut} />


Here oscalData will often contain fields other than what's returned in the REST response due to things like profile resolution. For example, SSPs will contain a resolvedControls array that should not be shown in source and should not be sent in the PUT REST request.

Handled the removal of resolvedControls in cf83ad1. If there are any others that need to be removed, please let me know. It's pretty opaque to me how those got in added to oscalData.

The way the additional fields are populated and passed in the resolvers (example) is currently a bit sloppy.

At some point we should revisit how we pass that additional info around to the components, but for now, perhaps we should consider shoving the original response string in something like oscalData.oscalSource rather than the approach of copying and removing what was added?

resolvedControls was just one example, modifications is another.

perhaps we should consider shoving the original response string in something like oscalData.oscalSource rather than the approach of copying and removing what was added?

Update layout of editor, hide `resolvedControls` attribute, and minor changes that improve testing. Tests were added in OSCALJsonEditor.test.js.

hreineck

UI looks great tested locally. Would like to see the toggle functionality as Ray described but other than that ready to merge.

kylelaker · 2022-03-01T14:13:49Z

@pkothare This merge conflict stemmed from #286 if that helps at all in the conflict resolution.

@rgauss

Based on @rgauss's comments, I am going to dismiss my review and withold approval until those issues are resolved.

This change adds the a `Fab` button that floats over the enitre viewer via a toolbar. Upon clicking the button, the JSON editor is toggled to either be displayed or hidden.

Preserved oscalSource and UI Changes

rgauss · 2022-03-02T17:40:23Z

@kylelaker, have your concerns been addressed here?

kylelaker · 2022-03-02T17:47:47Z

@rgauss I think so for the most part. I had previously approved and I think then dismissed because others had raised concerns. I haven't been following the developments on this branch super closely but I have no objections to merging it.

pkothare added 19 commits February 18, 2022 21:34

Add react-split library.

e34f277

Update OSCALLoader components.

ab0edb4

Added conditional checks for `isRestMode` and added the react-split control so that when `isRestMode` is `true` the `OSCALLoader` component will render a split pane.

Update App.js.

5f8a7a0

Add @monaco-editor/react pacakge.

8dc798f

Add OSCALJsonEditor.js.

3c2ccd7

Update OSCALLoader component.

e1d8f4e

Update App.js.

56a6c5b

The `maxWidth` value of `xl` was added which overrides the default of large. This was done so that the JSON editor can be accomodated in the application.

Update OSCALLoader.js.

844ad8f

The split pane defaults were updated so that the JSON Editor and viewer share a 50/50 split. The `minSize` value was also updated to 500 as a sensible default, because collapsing a pane to 0 can cause issues.

Update OSCALJsonEditor.js.

5666ce0

The updates improve the look and feel based on the margins and grid layout. The editor was also made sticky so that it follows veritical scrolling.

Update OSCALLoader.css.

54ea887

Removed overflow attributes, since they cause conflicts with sticky HTML elements.

Update package-lock.json.

3f73f94

Add OSCALJsonEditor.js.

6991288

Update OSCALLoader.js.

1aefb31

The split pane defaults were updated so that the JSON Editor and viewer share a 50/50 split. The `minSize` value was also updated to 500 as a sensible default, because collapsing a pane to 0 can cause issues.

Update OSCALLoader.js.

c6b0d4e

The split pane defaults were updated so that the JSON Editor and viewer share a 50/50 split. The `minSize` value was also updated to 500 as a sensible default, because collapsing a pane to 0 can cause issues.

Merge branch 'feature/json-editor' of github.com:EasyDynamics/oscal-r…

a6d767d

…eact-library into feature/json-editor

Merge remote-tracking branch 'origin/develop' into feature/json-editor

b737cf7

Add support for PUT method

80129de

Fix spacing issues

572ba46

Add OSCALLoaderStyles.js

a49f3a0

pkothare added 5 commits February 25, 2022 15:30

Update useLoaderStyles

b4d4e06

Update OSCALJsonEditor.js

138373e

Update OSCALLoader.js

e8cae87

Update OSCALLoaderForm.js

d1ac2d9

Update OSCALMetadata.js

b2e2f57

pkothare marked this pull request as ready for review February 25, 2022 21:23

pkothare requested review from rgauss, mikeisen1 and kylelaker February 25, 2022 21:23

pkothare requested review from hreineck and zclarkEDC February 25, 2022 21:23

kylelaker previously approved these changes Feb 26, 2022

View reviewed changes

rgauss suggested changes Feb 27, 2022

View reviewed changes

pkothare added 3 commits February 28, 2022 04:56

Update OSCALJsonEditor.js

cf83ad1

Update layout of editor, hide `resolvedControls` attribute, and minor changes that improve testing. Tests were added in OSCALJsonEditor.test.js.

Update OSCALLoader.js

298c17f

Update OSCALLoaderForm.js

926572c

hreineck approved these changes Mar 1, 2022

View reviewed changes

kylelaker mentioned this pull request Mar 1, 2022

Persist rest mode toggle across page loads #281

Merged

pkothare added 2 commits March 1, 2022 19:24

Merge remote-tracking branch 'origin/develop' into feature/json-editor

eb1d285

Update OSCALLoader.js

b9e70d3

This change adds the a `Fab` button that floats over the enitre viewer via a toolbar. Upon clicking the button, the JSON editor is toggled to either be displayed or hidden.

pkothare force-pushed the feature/json-editor branch from a833ac5 to b9e70d3 Compare March 2, 2022 02:12

Preserved oscalSource and UI Changes

7723e66

rgauss mentioned this pull request Mar 2, 2022

Preserved oscalSource and UI Changes #298

Merged

rgauss and others added 2 commits March 2, 2022 08:38

Fix linting

a966fe6

Merge pull request #298 from EasyDynamics/suggestion/json-editor-rgauss

28c3e72

Preserved oscalSource and UI Changes

rgauss approved these changes Mar 2, 2022

View reviewed changes

rgauss merged commit cecc1af into develop Mar 2, 2022

rgauss deleted the feature/json-editor branch March 2, 2022 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OSCALJsonEditor Component #265

Add OSCALJsonEditor Component #265

pkothare commented Feb 21, 2022 •

edited

kylelaker commented Feb 25, 2022 •

edited

pkothare commented Feb 25, 2022 •

edited

kylelaker left a comment •

edited by mikeisen1

pkothare commented Feb 27, 2022

rgauss Feb 27, 2022

pkothare Feb 28, 2022

rgauss Feb 28, 2022

rgauss Mar 1, 2022

hreineck left a comment

kylelaker commented Mar 1, 2022

rgauss commented Mar 2, 2022

kylelaker commented Mar 2, 2022

Add OSCALJsonEditor Component #265

Add OSCALJsonEditor Component #265

Conversation

pkothare commented Feb 21, 2022 • edited

TO DOs:

References

kylelaker commented Feb 25, 2022 • edited

pkothare commented Feb 25, 2022 • edited

kylelaker left a comment • edited by mikeisen1

Choose a reason for hiding this comment

pkothare commented Feb 27, 2022

rgauss Feb 27, 2022

Choose a reason for hiding this comment

pkothare Feb 28, 2022

Choose a reason for hiding this comment

rgauss Feb 28, 2022

Choose a reason for hiding this comment

rgauss Mar 1, 2022

Choose a reason for hiding this comment

hreineck left a comment

Choose a reason for hiding this comment

kylelaker commented Mar 1, 2022

rgauss commented Mar 2, 2022

kylelaker commented Mar 2, 2022

pkothare commented Feb 21, 2022 •

edited

kylelaker commented Feb 25, 2022 •

edited

pkothare commented Feb 25, 2022 •

edited

kylelaker left a comment •

edited by mikeisen1