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

fix(catalog): Support multi-level group & high-level withdrawn control anchor links #758

Merged
merged 20 commits into from
Apr 20, 2023

Conversation

Bronstrom
Copy link
Contributor

@Bronstrom Bronstrom commented Apr 7, 2023

This PR supports anchor links for multiple-levels of control groups.

Testing

Navigate to the Catalog Viewer and reload the following catalog(s):

Relates to #176

@Bronstrom Bronstrom changed the title feat(catalog): Support muli-level group anchors feat(catalog): Support muli-level group anchor links Apr 7, 2023
@Bronstrom Bronstrom marked this pull request as ready for review April 7, 2023 14:02
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 7, 2023 14:02
@tuckerzp
Copy link
Contributor

tuckerzp commented Apr 7, 2023

Is there sample data or a test to ensure this works? I remember some talk on this being tricky, but this also seems like something we'd prefer to not just trust it works.

At the very least discussion on why this can't be done would be good here. In the past, we have made dummy data to at least show a feature works even if we don't push it to our actual sample data.

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team April 7, 2023 15:48
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.

I think some changes in your previous pr to groups may have broken how we handle groups within groups.

Replication

https://oscal-viewer.msd.easydynamics.com/catalog/?url=https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/test-content/catalogs/basic-test-catalog.json

  1. Go to Test Group E
  2. Click on Test Group E.1

@kylelaker kylelaker changed the title feat(catalog): Support muli-level group anchor links fix(catalog): Support muli-level group anchor links Apr 10, 2023
group.title is ensured to be used when group.id
isn't included, including at the start of the
application running. Also state is better handled
at top level groupings in order to avoid any
unnecessary group opening/scrolling.
@Bronstrom
Copy link
Contributor Author

Doing some additional testing on this issue, I believe this is now in proper working order to fully support multi-level groups.

I think some changes in your previous pr to groups may have broken how we handle groups within groups.

Yes, when ensuring that a group.title is used when group.id isn't present I missed one place, and it would cause the application to fail right away when no group.id is provided by the first highest-level group. I've corrected this.

https://oscal-viewer.msd.easydynamics.com/catalog/?url=https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/test-content/catalogs/basic-test-catalog.json

Hey, this is actually a pretty good example to test multi-layered groups. Thanks for sharing this! I'll go ahead and place it in the description under "Testing".

@Bronstrom Bronstrom marked this pull request as draft April 11, 2023 19:22
Remaining state problems for groups and high level
controls have been resolved. An additional
state check to match the previous handled
fragment was found useful. Additionally upper-
level withdrawn controls were not appearing
properly due to a similar issue.
@Bronstrom Bronstrom force-pushed the brad/support-multi-level-group-anchor-links branch from 5972770 to 30d75bf Compare April 11, 2023 23:16
@Bronstrom
Copy link
Contributor Author

Bronstrom commented Apr 11, 2023

I've tried to stress test these groups and they seem to be working as intended. I also noticed that high-level withdrawn controls had some state issues in a similar vein, so I've made a fix to them as well.

Please refer to the test data provided above, when testing.

@Bronstrom Bronstrom marked this pull request as ready for review April 11, 2023 23:21
@Bronstrom Bronstrom changed the title fix(catalog): Support muli-level group anchor links fix(catalog): Support multi-level group & high-level withdrawn control anchor links Apr 12, 2023
@kylelaker
Copy link
Contributor

I also noticed that high-level withdrawn controls had some state issues in a similar vein, so I've made a fix to them as well

Are you talking about #743 or another issue?

@Bronstrom
Copy link
Contributor Author

Are you talking about #743 or another issue?

It's related to handling anchor link navigation for withdrawn controls. It's unrelated to opening a withdrawn control.

@kylelaker kylelaker requested a review from tuckerzp April 12, 2023 18:56
@tuckerzp tuckerzp linked an issue Apr 14, 2023 that may be closed by this pull request
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.

@Bronstrom I really appreacite all your hard work! These are some fairly complex changes, so my comments are mostly just about clarity. I think a bit of jsdoc and renaming could make things a bit easier to understand here.

@Bronstrom Bronstrom requested a review from tuckerzp April 17, 2023 20:30
@tuckerzp tuckerzp merged commit 3df7310 into develop Apr 20, 2023
6 checks passed
@tuckerzp tuckerzp deleted the brad/support-multi-level-group-anchor-links branch April 20, 2023 15:06
@kylelaker kylelaker added the bug Something isn't working label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Anchor Links
3 participants