feat(RHINENG-24161): align PolicyTypeTable cells vertically#2849
Closed
opacut wants to merge 1 commit into
Closed
Conversation
Reviewer's GuideWraps PolicyTypeTable in a container that applies middle vertical alignment to table cells via inline styles and new TableToolsTable props, ensuring text and radio buttons are vertically centered. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Avoid rendering a
<style>tag inside the component on every render; instead, move the.pf-v6-c-table__check { vertical-align: middle; }rule into a shared stylesheet or CSS module and reuse the existingpolicy-type-tableclass or a new className for scoping. - The wrapper
<div className="policy-type-table">is currently only used to host the inline<style>tag; once the styling is moved to a stylesheet, consider removing the wrapper to avoid unnecessary DOM nesting unless you need it for layout. - You are now setting vertical alignment both via the
.pf-v6-c-table__checkrule andtableBodyProps={{ style: { verticalAlign: 'middle' } }}; consider consolidating to a single, clearly scoped approach to avoid confusion and potential style conflicts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid rendering a `<style>` tag inside the component on every render; instead, move the `.pf-v6-c-table__check { vertical-align: middle; }` rule into a shared stylesheet or CSS module and reuse the existing `policy-type-table` class or a new className for scoping.
- The wrapper `<div className="policy-type-table">` is currently only used to host the inline `<style>` tag; once the styling is moved to a stylesheet, consider removing the wrapper to avoid unnecessary DOM nesting unless you need it for layout.
- You are now setting vertical alignment both via the `.pf-v6-c-table__check` rule and `tableBodyProps={{ style: { verticalAlign: 'middle' } }}`; consider consolidating to a single, clearly scoped approach to avoid confusion and potential style conflicts.
## Individual Comments
### Comment 1
<location path="src/PresentationalComponents/PolicyTypesTable/PolicyTypeTable.js" line_range="32-33" />
<code_context>
- label: 'Policy name',
- filterAttribute: 'title',
+ <div className="policy-type-table">
+ <style>
+ {`
+ .pf-v6-c-table__check {
+ vertical-align: middle;
</code_context>
<issue_to_address>
**suggestion:** Avoid defining a <style> tag inside the component to prevent duplicated global CSS on each render.
Because this `<style>` tag lives inside the component, the same CSS is injected on every mount and the `.pf-v6-c-table__check` rule is applied globally. Please move this rule into a shared stylesheet (or CSS-in-JS) and scope it with `.policy-type-table .pf-v6-c-table__check` so it only affects this table.
Suggested implementation:
```javascript
<div className="policy-type-table">
<TableToolsTable
```
1. Create or update a stylesheet for this component (for example `src/PresentationalComponents/PolicyTypesTable/PolicyTypeTable.scss` or `.css`) and add:
```css
.policy-type-table .pf-v6-c-table__check {
vertical-align: middle;
}
```
2. Import that stylesheet in `PolicyTypeTable.js` near the other imports, following the existing convention in this codebase. For example, if SCSS is used:
```js
import './PolicyTypeTable.scss';
```
3. Ensure the build setup is configured to process the chosen stylesheet type (CSS/SCSS), consistent with how other presentational components in the project are styled.
</issue_to_address>
### Comment 2
<location path="src/PresentationalComponents/PolicyTypesTable/PolicyTypeTable.js" line_range="60-68" />
<code_context>
+ emptyRows: emptyRows('policy types', columns.length),
+ }}
+ variant="compact"
+ tableBodyProps={{ style: { verticalAlign: 'middle' } }}
+ />
+ </div>
</code_context>
<issue_to_address>
**suggestion:** Unify the vertical alignment approach to avoid overlapping sources of truth.
There’s now both a CSS rule for `.pf-v6-c-table__check` and an inline `tableBodyProps` style (`verticalAlign: 'middle'`). Please pick one approach (ideally the CSS rule) to avoid maintaining two potentially conflicting alignment settings.
```suggestion
options={{
detailsComponent: PolicyTypeDetailsRow,
onRadioSelect: (_event, _value, _rowIdx, { item: { itemId } }) =>
onChange && onChange(profiles.find(({ id }) => id === itemId)),
emptyRows: emptyRows('policy types', columns.length),
}}
variant="compact"
/>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+32
to
+33
| <style> | ||
| {` |
There was a problem hiding this comment.
suggestion: Avoid defining a <style> tag inside the component to prevent duplicated global CSS on each render.
Because this <style> tag lives inside the component, the same CSS is injected on every mount and the .pf-v6-c-table__check rule is applied globally. Please move this rule into a shared stylesheet (or CSS-in-JS) and scope it with .policy-type-table .pf-v6-c-table__check so it only affects this table.
Suggested implementation:
<div className="policy-type-table">
<TableToolsTable-
Create or update a stylesheet for this component (for example
src/PresentationalComponents/PolicyTypesTable/PolicyTypeTable.scssor.css) and add:.policy-type-table .pf-v6-c-table__check { vertical-align: middle; }
-
Import that stylesheet in
PolicyTypeTable.jsnear the other imports, following the existing convention in this codebase. For example, if SCSS is used:import './PolicyTypeTable.scss';
-
Ensure the build setup is configured to process the chosen stylesheet type (CSS/SCSS), consistent with how other presentational components in the project are styled.
Comment on lines
+60
to
+68
| options={{ | ||
| detailsComponent: PolicyTypeDetailsRow, | ||
| onRadioSelect: (_event, _value, _rowIdx, { item: { itemId } }) => | ||
| onChange && onChange(profiles.find(({ id }) => id === itemId)), | ||
| emptyRows: emptyRows('policy types', columns.length), | ||
| }} | ||
| variant="compact" | ||
| tableBodyProps={{ style: { verticalAlign: 'middle' } }} | ||
| /> |
There was a problem hiding this comment.
suggestion: Unify the vertical alignment approach to avoid overlapping sources of truth.
There’s now both a CSS rule for .pf-v6-c-table__check and an inline tableBodyProps style (verticalAlign: 'middle'). Please pick one approach (ideally the CSS rule) to avoid maintaining two potentially conflicting alignment settings.
Suggested change
| options={{ | |
| detailsComponent: PolicyTypeDetailsRow, | |
| onRadioSelect: (_event, _value, _rowIdx, { item: { itemId } }) => | |
| onChange && onChange(profiles.find(({ id }) => id === itemId)), | |
| emptyRows: emptyRows('policy types', columns.length), | |
| }} | |
| variant="compact" | |
| tableBodyProps={{ style: { verticalAlign: 'middle' } }} | |
| /> | |
| options={{ | |
| detailsComponent: PolicyTypeDetailsRow, | |
| onRadioSelect: (_event, _value, _rowIdx, { item: { itemId } }) => | |
| onChange && onChange(profiles.find(({ id }) => id === itemId)), | |
| emptyRows: emptyRows('policy types', columns.length), | |
| }} | |
| variant="compact" | |
| /> |
Member
|
@opacut In general we should avoid inline style tags, this can mess very badly with other things. We should also not attempt to bodge-fix this and first figure out why there is a misalignment in the TableToolsTable and fix it there, so others benefit from it as well. |
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
Decided to solve instead with a pr to patternfly-react: patternfly/patternfly-react#12451 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aligns table cells vertically to the middle in the PolicyTypeTable component by wrapping the table in a div with an inline style block.
TableToolsTablein a<div className="policy-type-table"><style>block to vertically align radio button cellstableBodyProps={{ style: { verticalAlign: 'middle' } }}to vertically align text cellsHow to test
🤖 Generated with Claude Code
Summary by Sourcery
Enhancements: