-
Notifications
You must be signed in to change notification settings - Fork 44
Add service name class to Insights components to fix styling #1055
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
base: develop
Are you sure you want to change the base?
Conversation
Reviewer's GuideEnsure each Insights component is scoped with its service name for CSS isolation by wrapping or augmenting existing containers with the appropriate class (advisor or vulnerability). Class diagram for updated container structure in Insights componentsclassDiagram
class ScalprumComponent
class RemediationModal
class IopRecommendationsPage {
+div.advisor
+ScalprumComponent
}
class IopInsightsTab {
+div.advisor
+ScalprumComponent
}
class CVEsHostDetailsTab {
+div.rh-cloud-insights-vulnerability-host-details-component.vulnerability
+ScalprumComponent
}
class CveDetailsPage {
+div.rh-cloud-cve-details-page.vulnerability
+ScalprumComponent
}
class IopRecommendationDetails {
+div.iop-recommendation-details-scalprum.advisor
+ScalprumComponent
}
class InsightsVulnerabilityListPage {
+div.rh-cloud-insights-vulnerability-page.vulnerability
+ScalprumComponent
}
class IopRecommendationsCell {
+span.rh-cloud-insights-recommendations-cell.advisor
+ScalprumComponent
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @leSamo - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `webpack/ForemanColumnExtensions/index.js:26` </location>
<code_context>
return (
- <span className="rh-cloud-insights-recommendations-cell">
+ <span className="rh-cloud-insights-recommendations-cell advisor">
<ScalprumComponent scope={scope} module={module} />
</span>
</code_context>
<issue_to_address>
Adding 'advisor' class to a span may have unintended inline element effects.
Check that the 'advisor' class styles are suitable for use on a span and won't cause layout issues.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -23,7 +23,7 @@ const IopRecommendationsCell = hostDetails => { | |||
const module = './RecommendationsCellWrapped'; | |||
|
|||
return ( | |||
<span className="rh-cloud-insights-recommendations-cell"> | |||
<span className="rh-cloud-insights-recommendations-cell advisor"> |
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.
issue: Adding 'advisor' class to a span may have unintended inline element effects.
Check that the 'advisor' class styles are suitable for use on a span and won't cause layout issues.
<div className="advisor"> | ||
<ScalprumComponent |
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.
cc @ColeHiggins2 This may change your locators or whatnot
What are the changes introduced in this pull request?
Multiple styling issues in Lightspeed are caused by apps not being wrapped in a container with class name same as their service name.
This is a practice used on HCC to isolate CSS so it does not bleed into other apps -- each app is automatically wrapped in a div with class name same as their service name and stylesheets are restricted to only apply within containers of their corresponding app.
Considerations taken when implementing this change?
--
What are the testing steps for this pull request?
Open Vulnerability app and expand any row inside the table. The expandable section title "CVE description" should be bold. If it's bold than the styles are being applied correctly.
Before
After
Summary by Sourcery
Scope CSS for Insights components by wrapping them in containers with service-specific class names
Enhancements: