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: /security/cis redesign #14732

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

mtruj013
Copy link
Contributor

@mtruj013 mtruj013 commented Feb 10, 2025

Done

QA

Issue / Card

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

@webteam-app
Copy link

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.32%. Comparing base (a7e769a) to head (2a7a565).
Report is 80 commits behind head on feature-security-bubble.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature-security-bubble   #14732   +/-   ##
========================================================
  Coverage                    72.32%   72.32%           
========================================================
  Files                          120      120           
  Lines                         3404     3404           
  Branches                      1174     1177    +3     
========================================================
  Hits                          2462     2462           
  Misses                         917      917           
  Partials                        25       25           

@mtruj013 mtruj013 changed the title Apply redesign feat: /security/cis redesign Feb 10, 2025
@mtruj013 mtruj013 force-pushed the feature-security-bubble branch from 2a9bcba to 74bf728 Compare February 10, 2025 14:13
@eliman11
Copy link

eliman11 commented Feb 10, 2025

Looks good to me, thank you! A couple quick comments but they're more visual- than UX-related, so I'll +1 for UX anyway.

  • The Kubernetes logos looks really pixelated, maybe there's an alternative @kim-isaac can suggest?

  • On smaller viewports when the link wraps to the next line, could we break the link onto a new line?

Screenshot 2025-02-10 at 15 22 11 Screenshot 2025-02-10 at 15 22 04

@kim-isaac
Copy link

kim-isaac commented Feb 10, 2025

Hi @mtruj013 Thanks for your work! I have a few comments including Elaine's suggestions. (Thanks @eliman11 !!)

For this section,
Screenshot 2025-02-10 at 18 30 50

  • Button should be a default style
  • Could you change the logo section to this? Sorry I chose wrong one

Also for this section in the mobile view,
Screenshot 2025-02-10 at 18 34 53

  • Could you break the line between the btn and link? Elaine already pointed it out, but I just brought it up again!

That's all from my side. Thanks again!

@kim-isaac
Copy link

hi @mtruj013 this all looks great, thanks for making the adjustments! I'll add a design +1

Copy link
Contributor

@britneywwc britneywwc left a comment

Choose a reason for hiding this comment

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

Looks great overall, left a few questions and some minor suggestions :)

templates/security/cis.html Outdated Show resolved Hide resolved
<div class="row--50-50">
<hr class="p-rule" />
<div class="col">
<div class="p-section--shallow">
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no shallow section wrapping on design for h2 and p, this can be removed

Copy link
Contributor Author

@mtruj013 mtruj013 Feb 12, 2025

Choose a reason for hiding this comment

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

I see it on both, or do you mean a different section? nvm, I see what you're saying now. The spacing underneath the p on the design isn't native, I have to wrap both elements in an additional shallow section in order to replicate it
image

templates/security/cis.html Outdated Show resolved Hide resolved
templates/security/cis.html Outdated Show resolved Hide resolved
@mtruj013
Copy link
Contributor Author

Thanks @britneywwc! Could you have another look?

Copy link
Contributor

@britneywwc britneywwc 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 making the changes, looks great! 😄

@mtruj013 mtruj013 merged commit 8827c23 into canonical:feature-security-bubble Feb 13, 2025
15 checks passed
@mtruj013 mtruj013 deleted the security-cis branch February 13, 2025 08:22
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.

5 participants