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

Display Parties Contact Information #576

Merged
merged 18 commits into from
Aug 29, 2022

Conversation

tuckerzp
Copy link
Contributor

@tuckerzp tuckerzp commented Aug 18, 2022

This adds a Contact dialog box to metadata parties. When clicked, it opens a dialog displaying all the parties contact information. This also rewrites the tests a bit to no longer have parent components of OSCALMetadata test functionality.

Testing file

https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/f03073fddf33b0d65d3d0e84a7ad76c74e7be6c9/system-security-plans/ssp-example.json gives a good example of party contact info to view

Reviewing / Things to look for

This pr seems a bit more complex than it actually is upon further investigation. Here is a list of what changes have been made for reviewing.

  • OSCALMetadata has new sub components being used to display part contact information.
  • The tests have been expanded upon, they are more imperative and verbose now.
  • The storybook has additional components. This is fairly repetitive. Running npm run build-storybook && npm run storybook will let you view it locally.
  • New sample data has been added to be used in the tests / storybook.

closes #405

@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 12ac3ed to 156d45c Compare August 19, 2022 13:58
@Bronstrom
Copy link
Contributor

Looks like you got a good start to this issue!

I think Card works well here to organize the parties. When given multiple parties, the outlines around the Cards look a little wonky. I would suggest to remove the outlines.

I like the idea of using CardActions for more information — I think it would make sense to wrap all Card content for a party in one. When looking at one of our OSCALCatalog examples, it looks like most of the metadata is displaying underneath the party title and Avatar with some "undefined" values. Make sure to validate this info, and hide what's undefined.

Also, I think we should still display the secondary information, which I believe was the role title, we had previously underneath the party name at all times, while the additional metadata appears after selecting the Cards "action" area.

@kylelaker
Copy link
Contributor

kylelaker commented Aug 22, 2022

Make sure to validate this info, and hide what's undefined

For consistency in the UI, I'd rather not hide sections. I think having "This contact has no phone numbers" or whatever (with better wording) would be good. And I think if there aren't any contact details, then we just don't provide the button.

See https://material.io/design/communication/empty-states.html. I am not sure if MUI provides helpers for this; however, it's still something we can work on building.

@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 5cc874b to 8dfc4f1 Compare August 22, 2022 14:38
@tuckerzp tuckerzp marked this pull request as ready for review August 22, 2022 14:38
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 22, 2022 14:39
Rather than having the parent components of OSCALMetadata test
functionality, the tests have been rewritten to be more clear on exactly
what is being tested.
@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 8dfc4f1 to 20d1f17 Compare August 22, 2022 14:57
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 22, 2022 15:23
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 22, 2022 16:54
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 22, 2022 16:57
@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 89e323e to a2cde86 Compare August 22, 2022 17:06
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 22, 2022 19:03
@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch 2 times, most recently from 42f6174 to 0cb9ca7 Compare August 22, 2022 20:55
@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 0cb9ca7 to 092ae5b Compare August 23, 2022 12:36
src/components/OSCALMetadata.js Outdated Show resolved Hide resolved
src/components/OSCALMetadata.js Outdated Show resolved Hide resolved
@kylelaker kylelaker added the enhancement New feature or request label Aug 23, 2022
@tuckerzp
Copy link
Contributor Author

@Bronstrom

I think the contact type (Addresses, Phone, and Email) would look nice if center aligned.

I am not sure how great it looks.

image

image

Co-authored-by: Bradley Fellstrom <35196021+Bronstrom@users.noreply.github.com>
@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 1f5b783 to dac2b42 Compare August 24, 2022 18:13
src/components/OSCALMetadata.js Outdated Show resolved Hide resolved
@tuckerzp tuckerzp force-pushed the feature/metadata-parties-section-rework branch from 2156fcc to 00325da Compare August 24, 2022 19:07
@tuckerzp tuckerzp requested review from a team, hreineck and Bronstrom August 25, 2022 14:05
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.

This looks really good and is so cool! At some point, we should support location-uuids (or whatever that field is called) but I think that can be in another ticket (can you work on writing one up).

Thank you!!!

@tuckerzp tuckerzp linked an issue Aug 29, 2022 that may be closed by this pull request
@kylelaker kylelaker dismissed stale reviews from Bronstrom and hreineck August 29, 2022 16:36

All requested changes were specific, not structural. Dismissing to allow future approvals to advance the PR.

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 29, 2022 16:37
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.

Like the choices that were made to the structure and the Contact Dialog box looks pretty slick!

@Bronstrom Bronstrom merged commit 01bad7b into develop Aug 29, 2022
@Bronstrom Bronstrom deleted the feature/metadata-parties-section-rework branch August 29, 2022 20:49
tuckerzp added a commit to EasyDynamics/oscal-editor-deployment that referenced this pull request Aug 30, 2022
kylelaker pushed a commit to EasyDynamics/oscal-editor-deployment that referenced this pull request Aug 31, 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.

Increase Size of Metadata Parties Section Display Parties Contact Information
4 participants