Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@zhuje: This pull request references OU-656 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughPatternFly v5→v6 updates across CSS tokens/selectors and component APIs, chart imports moved to victory, package dependency bumps, toolbar chips→labels migration, button icon prop refactors, modal import changes, and several small utility/typing cleanups. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zhuje The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@zhuje: This pull request references OU-656 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/cypress/views/utils.ts (1)
36-42:⚠️ Potential issue | 🟠 MajorVersion detection logic is inverted.
The token
--pf-t--global--font--size--mdis part of the PatternFly v6 token system (confirmed by widespread use throughout the codebase), yet the code returns'v5'when this token is found. This causes v6 environments to be incorrectly identified as v5.For accurate v5 detection, check for the v5 token format
--pf-v5-global--FontSize--mdinstead.Proposed fix
if (style.getPropertyValue('--pf-v6-global--FontSize--md')) { return 'v6'; - } else if (style.getPropertyValue("--pf-t--global--font--size--md")) { + } else if (style.getPropertyValue("--pf-v5-global--FontSize--md")) { return 'v5'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/utils.ts` around lines 36 - 42, The version-detection checks are inverted: treat the token "--pf-t--global--font--size--md" as v6 and the v5 token as "--pf-v5-global--FontSize--md"; update the conditional sequence that calls style.getPropertyValue(...) so that if "--pf-t--global--font--size--md" is present you return 'v6', if "--pf-v5-global--FontSize--md" is present you return 'v5', and keep the default return as 'v6' to preserve current behavior.web/src/components/filters/attribute-filter.tsx (1)
142-147:⚠️ Potential issue | 🟠 MajorAvoid mutating
filtersduring label deletion.On Line 143,
filters?.[attribute]?.delete(...)mutates current state before emitting the update. Clone first, then delete, to keep updates immutable and predictable.Suggested fix
const handleDeleteAttributeValue = (attribute: string) => (_category: string | ToolbarLabelGroup, chip: string | ToolbarLabel) => { - filters?.[attribute]?.delete(chip as string); + const nextValues = new Set(filters[attribute] ?? []); + nextValues.delete(chip as string); onFiltersChange?.({ ...filters, - [attribute]: new Set(filters[attribute]), + [attribute]: nextValues, }); };As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/filters/attribute-filter.tsx` around lines 142 - 147, The deletion handler mutates the existing filters by calling filters?.[attribute]?.delete(...); instead, create an immutable update: shallow-clone the filters object, clone the Set at key attribute (e.g., new Set(filters?.[attribute] ?? [])), perform delete on the cloned Set, assign it into the cloned filters object, and then call onFiltersChange with the new cloned object (reference the existing onFiltersChange, filters, and the attribute/chip variables in the arrow function to locate where to apply this change).web/src/components/logs-histogram.tsx (1)
228-234:⚠️ Potential issue | 🟡 MinorUpdate PF chart color tokens to v6 equivalents in
BrushHandleComponent.The component uses v5 tokens (
--pf-v5-chart-color-blue-300) while logs-histogram.css already uses v6 tokens, creating an inconsistency during the codebase migration.Suggested fix
<line x1={x} x2={x} y1={y} y2={height + y} - style={{ stroke: 'var(--pf-v5-chart-color-blue-300)', strokeDasharray: '5 3' }} + style={{ stroke: 'var(--pf-v6-chart-color-blue-300)', strokeDasharray: '5 3' }} /> <polygon points={`${x},${y} ${x - triangleSize},${y - triangleSize} ${x + triangleSize},${ y - triangleSize }`} - style={{ fill: 'var(--pf-v5-chart-color-blue-300)' }} + style={{ fill: 'var(--pf-v6-chart-color-blue-300)' }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/logs-histogram.tsx` around lines 228 - 234, BrushHandleComponent is using the v5 CSS token '--pf-v5-chart-color-blue-300' for both the line stroke and polygon fill; replace those occurrences with the v6 chart color token that logs-histogram.css uses (i.e., swap '--pf-v5-chart-color-blue-300' for the v6 equivalent) so the stroke style on the line element and the fill style on the polygon element inside BrushHandleComponent use the same v6 token.
🧹 Nitpick comments (1)
web/src/severity.ts (1)
3-3: RenamechartCyanColorto match the imported teal token.The local variable name now conflicts with the actual imported token (
chart_color_teal_200), which hurts readability and future maintenance.Suggested rename
-import chartCyanColor from '@patternfly/react-tokens/dist/esm/chart_color_teal_200'; +import chartTealColor from '@patternfly/react-tokens/dist/esm/chart_color_teal_200'; ... - case 'trace': - return chartCyanColor.value; + case 'trace': + return chartTealColor.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/severity.ts` at line 3, The imported token alias chartCyanColor in severity.ts should be renamed to reflect the actual token (teal) — e.g., change the import alias chartCyanColor to chartTealColor (or chartColorTeal200) and update every usage of chartCyanColor in this module (functions, exports, constants) to the new name so the identifier matches the imported chart_color_teal_200 token for readability and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/package.json`:
- Around line 48-49: The package.json currently pins "eslint" to a version that
conflicts with the `@typescript-eslint` v8 peer dependency; update the "eslint"
dependency value from "^8.44.0" to "^8.57.0" so it satisfies the peer
requirement of "@typescript-eslint/parser" and
"@typescript-eslint/eslint-plugin" (both 8.39.1) and prevents install/lint CI
failures.
In `@web/src/components/logs-table.css`:
- Around line 90-95: In .lv-plugin__table__row-info replace the invalid token
--pf-v6-global--Background-color--100 with a valid PatternFly v6 semantic
background token (for example use
--pf-t--global--background--color--secondary--default or another appropriate
--pf-t--global--background--color--[concept]--[variant] token) so the CSS uses a
supported PFv6 background variable.
In `@web/src/components/precision-time-picker.css`:
- Line 11: In precision-time-picker.css locate the padding-top declaration that
currently uses a quoted CSS variable ('var(--pf-t--global--spacer--xs)') and
remove the surrounding quotes so padding-top uses the CSS variable directly
(padding-top: var(--pf-t--global--spacer--xs)); this ensures the browser
evaluates the variable instead of treating it as a string literal.
In `@web/src/components/virtualized-logs-table.tsx`:
- Line 190: In the table element in virtualized-logs-table.tsx update the
invalid versioned PatternFly modifier classes in the className (currently using
pf-v5-m-compact and pf-v5-m-border-rows) to the unversioned modifiers
pf-m-compact and pf-m-border-rows, or alternatively switch to the component prop
approach (e.g., use variant="compact" on the Table/React component where used)
so the table rendering in the VirtualizedLogsTable (the element with the
className attribute shown) uses the correct PatternFly modifiers.
---
Outside diff comments:
In `@web/cypress/views/utils.ts`:
- Around line 36-42: The version-detection checks are inverted: treat the token
"--pf-t--global--font--size--md" as v6 and the v5 token as
"--pf-v5-global--FontSize--md"; update the conditional sequence that calls
style.getPropertyValue(...) so that if "--pf-t--global--font--size--md" is
present you return 'v6', if "--pf-v5-global--FontSize--md" is present you return
'v5', and keep the default return as 'v6' to preserve current behavior.
In `@web/src/components/filters/attribute-filter.tsx`:
- Around line 142-147: The deletion handler mutates the existing filters by
calling filters?.[attribute]?.delete(...); instead, create an immutable update:
shallow-clone the filters object, clone the Set at key attribute (e.g., new
Set(filters?.[attribute] ?? [])), perform delete on the cloned Set, assign it
into the cloned filters object, and then call onFiltersChange with the new
cloned object (reference the existing onFiltersChange, filters, and the
attribute/chip variables in the arrow function to locate where to apply this
change).
In `@web/src/components/logs-histogram.tsx`:
- Around line 228-234: BrushHandleComponent is using the v5 CSS token
'--pf-v5-chart-color-blue-300' for both the line stroke and polygon fill;
replace those occurrences with the v6 chart color token that logs-histogram.css
uses (i.e., swap '--pf-v5-chart-color-blue-300' for the v6 equivalent) so the
stroke style on the line element and the fill style on the polygon element
inside BrushHandleComponent use the same v6 token.
---
Nitpick comments:
In `@web/src/severity.ts`:
- Line 3: The imported token alias chartCyanColor in severity.ts should be
renamed to reflect the actual token (teal) — e.g., change the import alias
chartCyanColor to chartTealColor (or chartColorTeal200) and update every usage
of chartCyanColor in this module (functions, exports, constants) to the new name
so the identifier matches the imported chart_color_teal_200 token for
readability and maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d927662-57f1-4e7f-b6f8-cedf3bb95b69
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
web/cypress/views/utils.tsweb/package.jsonweb/src/attribute-filters.tsxweb/src/components/alerts/logs-alerts-metrics.cssweb/src/components/error-message.cssweb/src/components/error-message.tsxweb/src/components/filters/attribute-filter.cssweb/src/components/filters/attribute-filter.tsxweb/src/components/filters/search-select.tsxweb/src/components/log-detail.cssweb/src/components/logs-histogram.cssweb/src/components/logs-histogram.tsxweb/src/components/logs-metrics.tsxweb/src/components/logs-query-input.cssweb/src/components/logs-table.cssweb/src/components/logs-toolbar.cssweb/src/components/logs-toolbar.tsxweb/src/components/precision-time-picker.cssweb/src/components/stats-table.cssweb/src/components/time-range-select-modal.cssweb/src/components/time-range-select-modal.tsxweb/src/components/timezone-dropdown.tsxweb/src/components/toggle-button.cssweb/src/components/toggle-play.cssweb/src/components/toggle-play.tsxweb/src/components/virtualized-logs-table.tsxweb/src/e2e-tests-app.tsxweb/src/hooks/useLocalStorage.tsxweb/src/hooks/useRefWidth.tsweb/src/pages/logs-detail-page.tsxweb/src/pages/logs-dev-page.tsxweb/src/pages/logs-page.tsxweb/src/severity.tsweb/src/value-utils.ts
web/src/e2e-tests-app.tsx
Outdated
| </li> | ||
| <li className="pf-v5-c-nav__item"> | ||
| <Link className="pf-v5-c-nav__link" to="/monitoring/logs"> | ||
| <li className="pf-v6-c-nav__item"> |
There was a problem hiding this comment.
We might have trouble upgrading to higher versions. Is there a way to use actual pattern fly components to avoid hard coding classes?
There was a problem hiding this comment.
Updated to use actual PF components
web/package.json
Outdated
| "@patternfly/react-icons": "^5.4.2", | ||
| "@patternfly/react-table": "^5.4.15", | ||
| "@patternfly/react-virtualized-extension": "^5.1.0", | ||
| "@patternfly/patternfly": "^6.0.0", |
There was a problem hiding this comment.
This might not be the latest version that includes patches or security fixes. Let's check the latest versions we can use safely
There was a problem hiding this comment.
Updated package.json to latest version 6.4.0.
web/src/components/toggle-play.css
Outdated
| .lv-plugin__toggle-play.pf-v5-c-button { | ||
| background-color: var(--pf-v5-global--BackgroundColor--100); | ||
| border: var(--pf-v5-global--BorderWidth--lg) solid var(--pf-v5-global--BorderColor--100); | ||
| .lv-plugin__toggle-play.pf-v6-c-button { |
There was a problem hiding this comment.
If possible we should avoid using pf selectors. If a version is changed in the future we will have to change it again
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/components/logs-table.css (1)
90-95:⚠️ Potential issue | 🟠 MajorInvalid PF v6 background token still present.
--pf-t--global--background--color--100uses the v5-style numeric suffix which doesn't exist in PF v6. The semantic token format is--pf-t--global--background--color--[concept]--[variant]--[state].For a white/primary surface background, use:
.lv-plugin__table__row-info { font-size: var(--pf-t--global--font--size--xs); - background-color: var(--pf-t--global--background--color--100); + background-color: var(--pf-t--global--background--color--primary--default); --pf-v6-c-table--cell--PaddingBlockEnd: var(--pf-t--global--spacer--xs); --pf-v6-c-table--cell--PaddingBlockStart: var(--pf-t--global--spacer--xs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/logs-table.css` around lines 90 - 95, The CSS rule for .lv-plugin__table__row-info uses an invalid PF v6 token --pf-t--global--background--color--100; update it to the PF v6 semantic background token for the white/primary surface (e.g., replace the numeric token with the semantic token format such as --pf-t--global--background--color--[concept]--[variant]--[state]) so .lv-plugin__table__row-info uses the correct v6 semantic token for primary/white surface background; ensure the replacement follows PF v6 naming conventions and remove the old numeric token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/logs-table.css`:
- Around line 66-80: The CSS rule for
.lv-plugin__table__severity-critical::before currently uses the info text token;
change its background-color to the danger token to match standard severity
conventions (and be consistent with getSeverityColor in web/src/severity.ts) by
replacing var(--pf-t--global--text--color--status--info--default) with
var(--pf-t--global--icon--color--status--danger--default) (or the theme token
used for "danger"/red in your design system).
---
Duplicate comments:
In `@web/src/components/logs-table.css`:
- Around line 90-95: The CSS rule for .lv-plugin__table__row-info uses an
invalid PF v6 token --pf-t--global--background--color--100; update it to the PF
v6 semantic background token for the white/primary surface (e.g., replace the
numeric token with the semantic token format such as
--pf-t--global--background--color--[concept]--[variant]--[state]) so
.lv-plugin__table__row-info uses the correct v6 semantic token for primary/white
surface background; ensure the replacement follows PF v6 naming conventions and
remove the old numeric token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b72e578-fb49-43a3-a0a0-1efae77f4c4c
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
web/package.jsonweb/src/components/log-detail.cssweb/src/components/logs-table.cssweb/src/components/precision-time-picker.cssweb/src/components/toggle-play.cssweb/src/components/virtualized-logs-table.tsxweb/src/e2e-tests-app.tsx
✅ Files skipped from review due to trivial changes (1)
- web/src/components/virtualized-logs-table.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/precision-time-picker.css
- web/src/components/log-detail.css
- web/src/e2e-tests-app.tsx
- web/src/components/toggle-play.css
| .lv-plugin__table__severity-critical::before { | ||
| background-color: var(--pf-v5-global--palette--purple-500); | ||
| background-color: var(--pf-t--global--text--color--status--info--default); | ||
| } | ||
|
|
||
| .lv-plugin__table__severity-error::before { | ||
| background-color: var(--pf-v5-global--danger-color--100); | ||
| background-color: var(--pf-t--global--icon--color--status--danger--default); | ||
| } | ||
|
|
||
| .lv-plugin__table__severity-warning::before { | ||
| background-color: var(--pf-v5-global--warning-color--100); | ||
| background-color: var(--pf-t--global--icon--color--status--warning--default); | ||
| } | ||
|
|
||
| .lv-plugin__table__severity-info::before { | ||
| background-color: var(--pf-v5-global--palette--green-300); | ||
| background-color: var(--pf-t--global--color--status--success--default); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any design docs or comments explaining severity color choices
rg -n -i 'critical|severity' --type=md --type=txt .
# Check previous color values to understand if this was intentional
git log -p --follow -S 'severity-critical' -- 'web/src/components/logs-table.css' 2>/dev/null | head -50Repository: openshift/logging-view-plugin
Length of output: 55
🏁 Script executed:
# Read the CSS file to check for comments explaining the color choices
cat -n web/src/components/logs-table.css | head -90Repository: openshift/logging-view-plugin
Length of output: 3146
🏁 Script executed:
# Search for HTML/JSX files that use these severity CSS classes to understand context
rg -l 'severity-critical|severity-error|severity-warning|severity-info' --type=tsx --type=ts --type=jsx --type=html -A 2Repository: openshift/logging-view-plugin
Length of output: 100
🏁 Script executed:
# Search for any comments or documentation about severity levels in the codebase
rg -i 'severity.*color|color.*severity' -A 3 -B 3Repository: openshift/logging-view-plugin
Length of output: 2044
🏁 Script executed:
# Check PatternFly token usage patterns - look for how other components map severity
rg 'pf-t--global.*status.*info|pf-t--global.*status.*danger|pf-t--global.*status.*success' --type=css -B 2 -A 2Repository: openshift/logging-view-plugin
Length of output: 1184
🏁 Script executed:
# Read the complete severity.ts file to see the getSeverityColor implementation
cat -n web/src/severity.tsRepository: openshift/logging-view-plugin
Length of output: 2983
🏁 Script executed:
# Find where chartPurpleColor is defined
rg -n 'chartPurpleColor' -B 5 -A 5Repository: openshift/logging-view-plugin
Length of output: 1533
🏁 Script executed:
# Check if the CSS classes are actually being used in templates
rg -n 'severity-critical|severity-error|severity-warning|severity-info' --type=jsx --type=ts -B 2 -A 2Repository: openshift/logging-view-plugin
Length of output: 100
🏁 Script executed:
# Search for files that apply these CSS classes
rg -n 'lv-plugin__table__severity' -B 3 -A 3Repository: openshift/logging-view-plugin
Length of output: 2271
Fix critical severity color token mapping.
The CSS critical severity color uses --pf-t--global--text--color--status--info--default (blue) instead of --pf-t--global--icon--color--status--danger--default (red/danger). Standard log severity conventions map critical to danger/red. Additionally, this is inconsistent with the TypeScript getSeverityColor() function in web/src/severity.ts, which maps critical to chartPurpleColor. Align the CSS critical color with the TypeScript implementation or standard severity conventions.
Current mapping at lines 66-80:
.lv-plugin__table__severity-critical::before {
background-color: var(--pf-t--global--text--color--status--info--default);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/logs-table.css` around lines 66 - 80, The CSS rule for
.lv-plugin__table__severity-critical::before currently uses the info text token;
change its background-color to the danger token to match standard severity
conventions (and be consistent with getSeverityColor in web/src/severity.ts) by
replacing var(--pf-t--global--text--color--status--info--default) with
var(--pf-t--global--icon--color--status--danger--default) (or the theme token
used for "danger"/red in your design system).
|
@zhuje: This pull request references OU-656 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/components/logs-table.css (1)
66-68:⚠️ Potential issue | 🟠 MajorCritical severity is still using an info token
Line 67 maps
.lv-plugin__table__severity-critical::beforeto--pf-t--global--text--color--status--info--default, which misrepresents critical events visually.Suggested fix
.lv-plugin__table__severity-critical::before { - background-color: var(--pf-t--global--text--color--status--info--default); + background-color: var(--pf-t--global--icon--color--status--danger--default); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/logs-table.css` around lines 66 - 68, The CSS selector .lv-plugin__table__severity-critical::before is using the info token (--pf-t--global--text--color--status--info--default); update this to use the correct critical/danger token (e.g., the pattern used for high-severity styles such as --pf-t--global--text--color--status--danger--default or the project’s equivalent critical token) so critical events render with the proper visual styling; modify the rule in logs-table.css to reference that critical token instead of the info token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/logs-table.css`:
- Around line 42-45: The rule setting the PatternFly CSS variable
--pf-v6-c-table--cell--FontSize on .lv-plugin__table__resources is ineffective
because that element is a Split inside a TableData cell; either remove the
unused variable assignment or replace it by applying font-size directly (e.g.,
set font-size on .lv-plugin__table__resources) so the visual size takes effect;
update the .lv-plugin__table__resources block to remove the
--pf-v6-c-table--cell--FontSize line or to use a direct font-size declaration
instead.
---
Duplicate comments:
In `@web/src/components/logs-table.css`:
- Around line 66-68: The CSS selector
.lv-plugin__table__severity-critical::before is using the info token
(--pf-t--global--text--color--status--info--default); update this to use the
correct critical/danger token (e.g., the pattern used for high-severity styles
such as --pf-t--global--text--color--status--danger--default or the project’s
equivalent critical token) so critical events render with the proper visual
styling; modify the rule in logs-table.css to reference that critical token
instead of the info token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f0cbefa-10ad-43d9-8c61-d7d1ca06bbca
📒 Files selected for processing (1)
web/src/components/logs-table.css
| .lv-plugin__table .lv-plugin__table__resources { | ||
| --pf-v5-c-table--cell--FontSize: var(--pf-v5-global--FontSize--md); | ||
| margin-top: var(--pf-v5-global--spacer--xs); | ||
| --pf-v6-c-table--cell--FontSize: var(--pf-t--global--font--size--md); | ||
| margin-top: var(--pf-t--global--spacer--xs); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where lv-plugin__table__resources is applied (expect: non-Td component usage)
rg -n -C2 'className="lv-plugin__table__resources"' --type=tsx
# Verify the CSS variable is currently assigned in logs-table.css
rg -n -C2 '--pf-v6-c-table--cell--FontSize|lv-plugin__table__resources' web/src/components/logs-table.cssRepository: openshift/logging-view-plugin
Length of output: 562
🏁 Script executed:
#!/bin/bash
# First, find where lv-plugin__table__resources is used (check for tsx/ts/jsx/js files)
echo "=== Searching for lv-plugin__table__resources className usage ==="
rg 'className.*lv-plugin__table__resources' -g '*.tsx' -g '*.ts' -g '*.jsx' -g '*.js' -n -C 2
echo -e "\n=== Viewing logs-table.css around lines 42-45 ==="
# Check the file size first
wc -l web/src/components/logs-table.css
echo -e "\n=== Content around lines 42-45 ==="
sed -n '40,50p' web/src/components/logs-table.cssRepository: openshift/logging-view-plugin
Length of output: 988
🏁 Script executed:
#!/bin/bash
echo "=== Full context around Split component usage ==="
sed -n '190,210p' web/src/components/logs-table.tsx
echo -e "\n=== Search for the table cell structure (Td component) ==="
rg 'Td|TableBody|TableRow' -g '*.tsx' web/src/components/logs-table.tsx -n -B2 -A2 | head -50
echo -e "\n=== Check PatternFly imports to understand Split ==="
head -40 web/src/components/logs-table.tsx | grep -i importRepository: openshift/logging-view-plugin
Length of output: 2778
🏁 Script executed:
#!/bin/bash
echo "=== Search for usage of --pf-v6-c-table--cell--FontSize variable ==="
rg --multiline 'pf-v6-c-table--cell--FontSize' -g '*.css' -g '*.tsx' -g '*.ts'
echo -e "\n=== Search for where .lv-plugin__table__message is used/styled ==="
rg 'lv-plugin__table__message' -g '*.css' -g '*.tsx' -n -C3
echo -e "\n=== Check full logs-table.css for context on this rule ==="
sed -n '35,55p' web/src/components/logs-table.cssRepository: openshift/logging-view-plugin
Length of output: 1878
🏁 Script executed:
#!/bin/bash
echo "=== Check if --pf-v6-c-table is referenced anywhere else ==="
rg 'pf-v6-c-table' -g '*.css' -g '*.tsx' -g '*.ts' -g '*.html'
echo -e "\n=== Check if there are any PatternFly table overrides that might consume this ==="
rg 'ResourceLinkList' -g '*.tsx' -n -C5 | head -40
echo -e "\n=== Full rule block for lv-plugin__table__resources ==="
sed -n '42,47p' web/src/components/logs-table.cssRepository: openshift/logging-view-plugin
Length of output: 3204
Remove unused PatternFly table cell property or apply font-size directly
Line 43 sets --pf-v6-c-table--cell--FontSize on .lv-plugin__table__resources, but this is a Split component inside the TableData cell, not the cell itself. PatternFly's table CSS only respects --pf-c-table--cell--* properties on actual cell elements. Either set font-size directly on .lv-plugin__table__resources, or remove this unused variable.
🧰 Tools
🪛 Stylelint (17.5.0)
[error] 44-44: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/logs-table.css` around lines 42 - 45, The rule setting the
PatternFly CSS variable --pf-v6-c-table--cell--FontSize on
.lv-plugin__table__resources is ineffective because that element is a Split
inside a TableData cell; either remove the unused variable assignment or replace
it by applying font-size directly (e.g., set font-size on
.lv-plugin__table__resources) so the visual size takes effect; update the
.lv-plugin__table__resources block to remove the --pf-v6-c-table--cell--FontSize
line or to use a direct font-size declaration instead.
|
@zhuje: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@anpingli I've performed a manual smoke test and it is logged in jira. Feel free to evaluate. If you agree, please could you label it? Thank you! |
|
@zhuje: This pull request references OU-656 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@zhuje: This pull request references OU-656 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@anpingli can we override this test until we have it up to date considering patternfly6 changes? |
5 participants
JIRA
https://redhat.atlassian.net/browse/OU-656
PatternFly v6 Migration Doc
https://www.patternfly.org/get-started/upgrade/
Image
quay.io/jezhu/logging-view-plugin:ou565-mar30
Related PR
openshift/observability-operator needs updates to the compatibility matrix to handle the new image stream for OCP v4.22 which requires Patternfly v6
#358