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

feat: add generic display of properties #786

Merged
merged 36 commits into from
May 4, 2023

Conversation

Bronstrom
Copy link
Contributor

@Bronstrom Bronstrom commented Apr 20, 2023

This creates a generic OSCALProperties component and adds it to back matter, controls, and metadata.

Testing

Try out the following catalog in the Catalog Viewer/Editor: https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/brad/test-catalog-generic-properties/catalogs/generic_properties_test_calalog.json

Closes #718

This creates a generic OSCALProperties component and adds it
to back matter, controls, and metadata.
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.

Very early thoughts. There are a few design things not written here that we should talk through. I will try to summarize those and provide more info soon.

tuckerzp
tuckerzp previously approved these changes Apr 26, 2023
Copy link
Contributor

@tuckerzp tuckerzp left a comment

Choose a reason for hiding this comment

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

Just had a quick look over things as they are. I had a few comments.

I also am not sure I understand why namespaceOf can't be used instead of isNistNamespace. I didn't have a ton of time to think it over, so maybe I am missing something.

@tuckerzp tuckerzp dismissed their stale review April 26, 2023 19:03

Accidently hit approve

@Bronstrom
Copy link
Contributor Author

Just had a quick look over things as they are. I had a few comments.

I also am not sure I understand why namespaceOf can't be used instead of isNistNamespace. I didn't have a ton of time to think it over, so maybe I am missing something.

namespaceOf returns a namespace or the default NIST namespace when there is no namespace provided. isNistNamespace validates the namespace being a NIST namespace, which is true when ns is empty or an empty string. This validation check is done a few times within OSCALProperties and I thought it made sense to be a function. This could be turned into namespaceOf(ns) === NIST_DEFAULT_NAMESPACE if we wanted to use that function inside of this one.

@Bronstrom Bronstrom marked this pull request as ready for review April 26, 2023 22:49
@Bronstrom Bronstrom marked this pull request as draft April 26, 2023 23:12
@tuckerzp
Copy link
Contributor

tuckerzp commented Apr 28, 2023

I discussed this briefly with @Bronstrom, but I wonder if filtering out non nist namespaces to a separate list to only then put it in more or less the same list is the best approach.

To me, this feels more like we need to sort. Anything that is considered to be in the default namespace (found preferably using namespaceOf and a boolean check) would be put at the top of the list while everything else can just remain in the same order.

I honestly am unsure if this cleans up the code a ton, but conceptually it makes more sense to me in terms of what we are actually trying to do. I also think it will make the return statement of the component a lot cleaner. Feel free to ignore me if I am wrong here or this is too big of a change 😆

@kylelaker
Copy link
Contributor

To me, this feels more like we need to sort. Anything that is considered to be in the default namespace (found preferably using namespaceOf and a boolean check) would be put at the top of the list while everything else can just remain in the same order.

Are you suggesting something like:

function compareNamespace(a: Property, b: Property): number {
  if (namespaceOf(a) === namespaceOf(b)) {
    return 0;
  }
  if (namespaceOf(a) === NIST_DEFAULT_NAMESPACE) {
    return -1;
  }
  if (namespaceOf(b) === NIST_DEFAULT_NAMESPACE) {
    return 1;
  }
  return namespaceOf(a).localeCompare(namespaceOf(b));
}

const sorted = props
  .sort((a, b) => a.name.localeCompare(b.name))
  .sort(localeCompare);

That feels immensely complicated compared to filtering. And then we're just encoding that information in the sort order...

This fixes mertadata's title, which didn't
display the label correctly, as well as removes
the fallback in favor of having resources with
no title be handled in OSCALBackMatter.
One of the Profile tests continues to fail more, so the wait
time has been extended. Also some of the catalog group tests
don't expect children underneath the controls (like props),
so additional test data was created specifically with
props data for tests.
@Bronstrom
Copy link
Contributor Author

Here's some screenshots of how the modal is looking:

generic_properties_modal_1

generic_properties_modal_2

@Bronstrom Bronstrom marked this pull request as ready for review May 1, 2023 17:40
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.

Small iconography change and then I think I am good to move forward with merging.

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.

Oh! We also need a storybook file for this! At least for the OSCALProperties dialog itself

);
};

interface OSCALPropertiesDialogProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface OSCALPropertiesDialogProps {
export interface OSCALPropertiesDialogProps {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

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.

Thanks for your work on this!

@tuckerzp tuckerzp merged commit 8528974 into develop May 4, 2023
5 checks passed
@tuckerzp tuckerzp deleted the brad/add-generic-display-of-properties branch May 4, 2023 15:53
tuckerzp pushed a commit that referenced this pull request May 25, 2023
This creates a generic OSCALProperties component and adds it to back
matter, controls, and metadata.

## Testing

Try out the following catalog in the Catalog Viewer/Editor:
https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/brad/test-catalog-generic-properties/catalogs/generic_properties_test_calalog.json

Closes #718

---------

Co-authored-by: Kyle Laker <klaker@easydynamics.com>
@kylelaker kylelaker added the enhancement New feature or request label Jun 12, 2023
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.

Display properties for metadata, back matter, and controls
3 participants