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

Support Friendly URLs for documents #478

Merged
merged 3 commits into from
Jul 5, 2022

Conversation

tuckerzp
Copy link
Contributor

@tuckerzp tuckerzp commented Jun 29, 2022

Rather than loading by OscalURL's in the editor, we now load based on
the OSCALObjects uuid. This allows for a lot more coherent way of
routing throughout the editor. It also allows you to load an element by
placing the uuid in the url.

This may render some portions of the OSCALLoader redundant. As I still do not
feel completely confident on what everything in the OSCALLoader does, please
pay extra attention in case I missed something.

Testing still needs to be added to this, but the functionality should
be working. It makes more sense to deal with this testing in EasyDynamics/oscal-editor-deployment#98

Closes #336

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team June 29, 2022 18:14
Rather than loading by OscalURL's in the editor, we now load based on
the OSCALObjects uuid. This allows for a lot more coherent way of
routing throughout the editor. It also allows you to load an element by
placing the uuid in the url.
@tuckerzp tuckerzp force-pushed the feature/friendly-urls-in-viewer branch from 4051f06 to d525a5a Compare June 29, 2022 18:15
Copy link
Contributor

@kylelaker kylelaker left a comment

Choose a reason for hiding this comment

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

On the whole... wow! Love it! I think there are some small adjustments we can make to reduce duplication further.

example/src/App.js Outdated Show resolved Hide resolved
example/src/App.js Outdated Show resolved Hide resolved
src/components/OSCALLoaderForm.js Outdated Show resolved Hide resolved
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team June 29, 2022 19:34
@kylelaker kylelaker changed the title Support Friendly URLS Support Friendly URLs Jun 29, 2022
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team June 29, 2022 20:49
@kylelaker
Copy link
Contributor

Okay so it looks like there is an odd behavior here.

Steps to Reproduce

  1. Launch the editor, go to any OSCAL Document type (for this example, Catalog)
  2. Choose a catalog from the drop down
  3. Wait for the catalog to load (or don't)
  4. Go to the navigation
  5. Click the same OSCAL document type (in this example, Catalog)

Expected Behavior

The page is blank except for the LoaderForm.

Actual Behavior

The previous document remains on the page.

@tuckerzp
Copy link
Contributor Author

Expected Behavior
The page is blank except for the LoaderForm.

Actual Behavior
The previous document remains on the page.

I agree that it doing that would make sense but I am not 100% sure this is an issue that needs to be solved here. #472 will get rid of the ability to click on a OSCALType like that. So going to just catalog will no longer be possible. Making this behavior impossible to replicate.

With that said, I also think that what we push to develop should be a working version of the editor. But it seems that Develop also has this same behavior. Although this branch does remove the OSCALLoaderForms data.

I am fully open to working on fixing this, I just wanted to be sure that it is something worth fixing on this pr.

@Bronstrom
Copy link
Contributor

Expected Behavior
The page is blank except for the LoaderForm.

Actual Behavior
The previous document remains on the page.

I agree the actual behavior is odd, but with it being carried over from develop it seems more logical to fix in a separate PR. Although, I lean towards skipping over addressing this issue with #472 already in development and changing the user interaction of selecting OSCAL documents.

example/src/App.js Outdated Show resolved Hide resolved
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team July 5, 2022 13:39
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team July 5, 2022 16:06
@kylelaker
Copy link
Contributor

@tuckerzp Please ensure that all supporting PRs on other repos are merged before we merge this in. Tests in oscal-editor-deployment aren't strictly necessary but I don't want regressions in the REST service or Docker container because of this PR

Copy link
Contributor

@Bronstrom Bronstrom left a comment

Choose a reason for hiding this comment

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

This is looking great to me and I like the choices made to limit redundancy with the app bar and navigation routes! It appears all supporting PRs (titled as "Support Friendly URLs" across other repos) have been merged, so I will go ahead and merge this PR.

@Bronstrom Bronstrom merged commit 08e4c1d into develop Jul 5, 2022
@Bronstrom Bronstrom deleted the feature/friendly-urls-in-viewer branch July 5, 2022 17:27
@kylelaker kylelaker changed the title Support Friendly URLs Support Friendly URLs for documents Aug 29, 2022
@kylelaker kylelaker added the enhancement New feature or request label Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Friendly/Semantic URLs in Viewer
3 participants