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

RSC-1770: Vulnerability Counter, RSC-1771: Small Vulnerability Counter #1200

Open
wants to merge 75 commits into
base: FEATURE_VULNERABILITY_COMPONENTS
Choose a base branch
from

Conversation

…rability_counter

# Conflicts:
#	lib/src/base-styles/_nx-colors.scss
…_threat_counter

# Conflicts:
#	lib/src/util/vulnerabilityLevels.ts
…_threat_counter

# Conflicts:
#	gallery/src/jsUtilPages/VulnerabilityThreatLevelUtils/GetVulnerabilityLevelCategoryExample.ts
Comment on lines 184 to 186
'Vulnerability Counter': { content: NxVulnerabilityCounterPage, type: 'react' },
'Small Threat Counter': { content: NxSmallThreatCounterPage, type: 'react' },
'Small Vulnerability Counter': { content: NxSmallVulnerabilityCounterPage, type: 'react' },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not commingle these with the Threat-related components. Move them to the bottom of the list where "V" would order naturally.

Copy link
Author

Choose a reason for hiding this comment

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

Modified the list here: d2833e7

Copy link
Contributor

Choose a reason for hiding this comment

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

Modified the list here: d2833e7

They should be after Overflow Tooltip

@rpokorny
Copy link
Contributor

Please add tech debt tickets to reduce the code duplication introduced here. Idk if we'll ever get to them but we should record it.


await runTimers();

const counter = component.getByTitle('Vulnerability: Critical (9.0\u201410.0)');
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace the escape with the literal throughout.

Copy link
Author

Choose a reason for hiding this comment

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

Replaced here: 57fd872

Copy link
Contributor

Choose a reason for hiding this comment

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

Replaced here: 57fd872

There's more of them; search the file.

@mixin counter-colors {
@each $severity in critical, high, medium, low, none {
&--#{$severity} {
background-color: var(--nx-color-vulnerability-#{$severity});
Copy link
Contributor

Choose a reason for hiding this comment

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

The rendered color here doesn't match the spec, at least for critical (I haven't checked the others yet). Are the color variables correct? Is this dependent on the changes in #1197 ?

}

.nx-small-vulnerability-counter {
$height: 25px;
Copy link
Contributor

@rpokorny rpokorny Oct 11, 2024

Choose a reason for hiding this comment

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

This value does't match the spec. The counters appear to only be 21px tall. Additionally you might not need to set the height explicitly, it'd be better to see if you can get it to just work out naturally with a careful choice of line-height. Be sure to take the borders into account - with your current styles the counters are actually 27px tall.

@use "../../scss-shared/nx-text-helpers";

@mixin counter-colors {
@each $severity in critical, high, medium, low, none {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see designs for the "none" vuln ratings level in figma.


.nx-small-vulnerability-counter-container {
display: inline-flex;
gap: var(--nx-spacing-2x);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 4 not 8

Comment on lines +57 to +58
margin-left: var(--nx-spacing-2x);
margin-right: var(--nx-spacing-2x);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a spec for this though I do see that it matches NxSmallThreatCounter.

width: 100%;

// using the counter-colors mixin will override the zero styling in this use case
@include counter-colors;
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec doesn't call this out. Check with @JasonDSwearingen that this is desired for this component just like NxSmallThreatCounter.

@use '../../scss-shared/nx-text-helpers';
@use '../../scss-shared/nx-dark-mode-helpers';
@use "../../scss-shared/nx-text-helpers";
@use "../../scss-shared/nx-dark-mode-helpers";
Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous change

@rpokorny
Copy link
Contributor

The mockup linked in the ticket covers NxSmallVulnerabilityCounter but still not NxVulnerabilityCounter, as far as I'm seeing.

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

Successfully merging this pull request may close these issues.

5 participants