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: Create a template for individual vulnerability pages #14708

Open
wants to merge 10 commits into
base: vulnerability-knowledge-base
Choose a base branch
from

Conversation

petesfrench
Copy link
Contributor

@petesfrench petesfrench commented Feb 4, 2025

Done

QA

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-17114

@webteam-app
Copy link

@petesfrench
Copy link
Contributor Author

Hey @eliman11, this is the vulnerabilities page we spoke about. You can see it at this demo link. I am open to any suggestions.

Bare in mind we don't have full control over the content. But we do get 'chunks' of content that we can put where we want. The screenshot below indicate what the 'chunks' are with a red box. You will notice it is a series of 'titles' and then the associated 'content'.

vulnerabilities_page

@britneywwc
Copy link
Contributor

  • Check the appropriate notifications appear on /security (should just be the retbleed one above

I don't see the retbleed notification on /security page, just these:
Screenshot 2025-02-06 at 4 16 19 PM

@britneywwc
Copy link
Contributor

On medium and small screens, there is some white padding on the right side of the page
Screenshot 2025-02-06 at 4 20 14 PM
Screenshot 2025-02-06 at 4 20 41 PM

The table of content is also hidden, I assume there should be a "Toggle table of contents" button but it is not shown either

Comment on lines +13 to +15
<div class="u-fixed-width">
<a href="/security/vulnerabilities" class="p-link--soft">&lang;&nbsp;&nbsp;&nbsp;Back to all vulnerabilities</a>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the Vanilla breadcrumb component here?

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 shout! The repeating &nbsp; did feel a little hacky

@eliman11
Copy link

eliman11 commented Feb 6, 2025

Thanks for all your work on this @petesfrench, it looks great! Just one more tiny adjustment I promise - can we change the wording on the button into "> Open side navigation" rather than "> Toggle side navigation" and "< Close side navigation" to collapse it on smaller screens? I'll add UX+1 anyway though - thank you again!

toggleButtonOutsideDrawer.setAttribute("aria-expanded", "false");
toggleButtonInsideDrawer.setAttribute("aria-expanded", "false");
toggleButtonOutsideDrawer?.setAttribute("aria-expanded", "false");
toggleButtonInsideDrawer?.setAttribute("aria-expanded", "false");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion here would be to add some helper classes to reduce redundancy and improve readability.

For example we can have toggleSideNavClass(addClass, removeClass) for lines 25-26 and 34-35.
We can also have another helper class for setting aria-expanded i.e setAriaExpanded(expanded) to handle lines 31-32 and 40-41.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants