-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add support for anchor links #600
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very early thoughts
I am taking this out of review since the functionality is mostly there. We need #88 to be in before we can be sure things work perfectly. |
I have realized that how we deal with scrolling to section headers will not work with our controls listed in the catalog. We will need a more specialized version of As an initial implementation, I thought it may be helpful to use a regex to check if a hash is a control id. Here is an initial attempt: |
After a few months I've returned back to this issue with an imperfect and hacky solution to help solve navigation to controls from an anchor hash. The main issue is that the controls need to be navigated to by selecting their control grouping once loaded, otherwise they are completely skipped over when using One idea I had was to interact with the collapsible states of the control groupings, but it seems this would get fairly messy with the control groupings being buried deep within the components and not easily interactable during the loading stage. I ended up choosing to adjust the IDs for the control grouping tabs and interacting with the DOM by using the Let me know if anyone has a better idea for solving this. |
@Bronstrom I am glad to see this worked on a bit! Would it be best to convert this to a draft for now? |
To avoid errors from occurring in the console, hashes for controls contained within control group tabs are dealt with at a lower level than `OSCALLoader`.
Removing the in-line styling here, appears to have no effect on the general style overall and displays an anchor link properly.
Previously an implementation was planned to move where the control was placed in OSCALCatalogGroup, but was instead duplicated rather than moved. This fixes this and removes browser warnings about duplicated controls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave this a quick glance over. This is looking really good and I appreciate your hard work! I do have a few comments.
I noticed a lot of places where we have created functions just checking that something is/ is not null
or ""
. From my understanding, this can be done just with a boolean check as both values are falsy. I think it would clean up a bit of the indirection we have happening here.
It is a bit late for it now, but I think more conversation on what is meaningful to add a anchor link to would have been useful. I don't want to add a link to everything just because we can.
…s/oscal-react-library into feature/support-anchor-links
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am content with the general format and structure of this. My approval is based on the assumption that the remaining comments are addressed and that we get that small test failure cleaned up.
Thank you for your work on this. @tuckerzp please keep an eye on this and provide the final review.
Multi-level groups are now supported! We may need to do some tweaking to this later, once we have some real test data content with fleshed out groupings, but from the test data I was able to input into the content of our editor this works properly. |
This reverts commit 318998b.
It was decided we would do some more stress testing on this before this change is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to move forward with this to reduce the number of times we have to resolve merge conflicts because of #722. Thank you @Bronstrom for your work.
We expect two follow up PRs: one to handle back matter resource links and another to fix up some of the support for layers of groups.
This PR adds Cypress tests ensuring EasyDynamics/oscal-react-library#600 satisfies the desired properties of anchor links. These tests ensure that a visited url with a valid hash will navigate to a part of the page and a url contains the correct hash after an anchor link is clicked. Furthermore, anchor links contained within control grouping are used in these test cases as they are the "most buried" anchor links within tab layers, making them the most difficult to test. Resolves EasyDynamics/oscal-react-library#176 --------- Co-authored-by: Kyle Laker <klaker@easydynamics.com> Co-authored-by: Zachary Tucker <zachptucker@gmail.com>
With the addition of anchor links to OSCAL Viewer/Editor users can quickly navigate between controls and sections of an OSCAL document.
In order to implement this, the class:
OSCALAnchorLinkHeader
was created to create an "anchored" header. This class sets the headers id to a specific id value when provided (or converts the header title), and makes the header hoverable to make an anchor link icon appear. Our anchor links work very similarily to heading in Microsoft Docs. The Loader was also updated to scroll directly to each "anchored" header, when a hash is specified in the browser URL and the hash matches an "anchored" header. This occurs specifically on hash updates and after reloads.Testing
/system-security-plan/?url=https://raw.githubusercontent.com/EasyDynamics/oscal-demo-content/main/system-security-plans/ssp-example.json#authorization-boundary
Closes #176