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

Improve Catalog Control Group Navigation #602

Merged
merged 14 commits into from
Sep 16, 2022

Conversation

tuckerzp
Copy link
Contributor

@tuckerzp tuckerzp commented Sep 2, 2022

This uses Tabs from material ui to make a vertical menu to navigate catalog control groups. It also extracts sub groups to its own function.

closes #88

@kylelaker kylelaker changed the title Improved Catalog Control Group Navigation Improve Catalog Control Group Navigation Sep 5, 2022
@kylelaker kylelaker added the enhancement New feature or request label Sep 5, 2022
@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch from 2a10b3b to 191b370 Compare September 6, 2022 18:13
@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch from 191b370 to 5791be5 Compare September 6, 2022 18:53
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.

I think we're pretty significantly missing a design for groups in groups in groups. And we're possibly changing the behavior of a control in a control.

The names of these components are also starting to imply that maybe we're losing a little insight into the spec. Remember that it's also possible to have controls without groups (in additions to just layers of groups all the way down).

src/components/OSCALControlPart.js Outdated Show resolved Hide resolved
src/components/OSCALCatalogGroup.js Outdated Show resolved Hide resolved
src/components/OSCALCatalogGroups.js Show resolved Hide resolved
src/components/OSCALCatalogGroups.js Outdated Show resolved Hide resolved
@tuckerzp
Copy link
Contributor Author

tuckerzp commented Sep 7, 2022

I think we're pretty significantly missing a design for groups in groups in groups

I do not think we have ever tested that so I created https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/nested-groups/catalogs/NIST_SP-800-53_rev4_catalog.json. Things do get very broken. Although I also do not like how the OSCALControlParamLegend is displayed for each group on our current viewer. I will rework this to accommodate multiple groups.

@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch from 2ecbf8f to daeb39c Compare September 7, 2022 17:47
@tuckerzp tuckerzp marked this pull request as ready for review September 7, 2022 17:53
@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch 2 times, most recently from 9b2e2b5 to e20afd6 Compare September 7, 2022 19:21
@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch from e20afd6 to 5760511 Compare September 7, 2022 19:29
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.

Really good progress here! Thanks! I think there are just a few more things I'd like to try to implement before merging this.

src/components/OSCALCatalogGroups.js Outdated Show resolved Hide resolved
src/components/OSCALCatalogGroups.js Show resolved Hide resolved
@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch from d113fb4 to 4676625 Compare September 12, 2022 13:30
@tuckerzp tuckerzp force-pushed the feature/improved-catalog-control-group-navigation branch from 42dc407 to 7e279e9 Compare September 12, 2022 13:44
@kylelaker
Copy link
Contributor

@tuckerzp Do you think that you can provide a summary of the status of this PR? Is it ready for review again or are there additional things that need to be completed?

Copy link
Contributor

@hreineck hreineck 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! Just tested it locally and the new UI works great.

src/components/OSCALCatalogGroup.js Outdated Show resolved Hide resolved
groupKey adds unneeded complexity to hash the title of groups as a key.
group.title is required and unique according to the spec, so it can
safely be used as a key.
@tuckerzp
Copy link
Contributor Author

@kylelaker

provide a summary of the status of this PR?

I think functionality wise, everything is good to go. It is mostly stylistic issues brought up I need to address.

Try and to text-alight: left or something else reasonable (this may require padding/margin)

I know that the issue has this happening, but I am now not sure if that would be best. I just realized that after clicking on a tab and then using the up/down arrow keys, the currently selected box gets a really neat highlight feature. It is highlighted on the center of the tab, so I feel like having the text centered makes sense.

Any reason we can't use a counter? Or set the key to equal the title itself?

I got rid of groupKey completely.

Have line in the middle go all the way down

The issue I am having here is that sometimes the tabs are longer and sometimes the content is longer. Currently the line is a borderRight of the tabs. I tried adding a borderLeft and it does fix the issue when the content is longer. The issue is that these two lines render slightly differently, meaning I cant use both.

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.

While I don't think that this necessarily handles every edge case that we could throw at it, I do think it's a pretty nifty start. Thanks @tuckerzp. I am content with the design and the structure. You've put a lot of good effort into this and I appreciate it!

@kylelaker kylelaker dismissed hreineck’s stale review September 14, 2022 01:58

The specific issue highlighted has been addressed and "Request Changes" will block another review from approving/merging.

@nuttercd
Copy link
Contributor

@tuckerzp This is really good work! I think the currently selected box highlighting feature is very cool.

@zclarkEDC zclarkEDC merged commit 27a11cb into develop Sep 16, 2022
@zclarkEDC zclarkEDC deleted the feature/improved-catalog-control-group-navigation branch September 16, 2022 19:52
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.

Improved Catalog Control Group Navigation
5 participants