OCPBUGS-65812: Layout issue on YAML editor settings popup window #15753
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: XiyunZhao The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughApplied inline layout styles to the edit YAML settings modal to prevent wrapping: set Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/modals/edit-yaml-settings-modal.tsx
🔇 Additional comments (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
85-85: LGTM – Low-risk alignment fix.The
whiteSpace: 'nowrap'style on the SplitItem containing the Switch prevents the toggle label from wrapping, maintaining proper alignment. Since the labels are just "On"/"Off" (very short text), the risk of overflow is minimal.
| <Flex | ||
| alignItems={{ default: 'alignItemsCenter' }} | ||
| spaceItems={{ default: 'spaceItemsMd' }} | ||
| style={{ flexWrap: 'nowrap' }} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
Verify responsive behavior on narrow viewports.
The flexWrap: 'nowrap' inline style fixes the reported layout issue by preventing the icon and text from wrapping. However, on narrow viewports or small screens, this could cause horizontal overflow if the description text is lengthy.
Please test the modal on various screen sizes (mobile, tablet) to ensure the content doesn't overflow or cause horizontal scrolling. Consider whether the modal needs a minimum width or if text truncation with ellipsis would be a better fallback for very narrow screens.
🏁 Script executed:
#!/bin/bash
# Verify if PatternFly's Modal component with variant="small" has responsive handling
# Search for Modal usage patterns and responsive utilities in the codebase
rg -nP --type=tsx --type=ts -C3 'Modal.*variant.*small|pf-v6-u-text-truncate|pf-v6-u-text-nowrap' -g '!node_modules' -g '!dist'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Correct search using glob patterns for tsx/ts files
# 1. Examine the actual file being modified
echo "=== Examining the modified file ==="
cat "frontend/public/components/modals/edit-yaml-settings-modal.tsx" | head -120 | tail -60
echo -e "\n=== Searching for Modal variant patterns ==="
rg -nP -g '*.tsx' -g '*.ts' 'Modal.*variant' -C2 | head -50
echo -e "\n=== Searching for flexWrap or nowrap patterns ==="
rg -nP -g '*.tsx' -g '*.ts' 'flexWrap|nowrap' -C2 | head -50Length of output: 9266
🏁 Script executed:
#!/bin/bash
# Check PatternFly documentation for responsive Modal behavior
# This queries external resources about PatternFly Modal component
echo "Checking PatternFly Modal responsive behavior patterns..."
curl -s "https://www.patternfly.org/components/modal" 2>/dev/null | grep -i "responsive\|small\|width" | head -20 || echo "Could not fetch external documentation"Length of output: 57
🌐 Web query:
PatternFly Modal variant small responsive width behavior
💡 Result:
Short answer: PatternFly's "small" modal is implemented as a responsive max-width, not a fixed pixel width. By default the modal box width is calc(100% - spacer) and each variant sets a max‑width; the small variant's max‑width is 35rem (so it will shrink on narrow viewports and center until it reaches that max). You can override with the Modal's width prop or by changing the --pf-c-modal-box--m-sm--sm--MaxWidth (or the general --pf-c-modal-box--MaxWidth) CSS variable. [1][2]
Sources:
- PatternFly Modal docs and CSS variables (shows default width and --pf-c-modal-box--m-sm--sm--MaxWidth = 35rem). [1]
- PatternFly Elements / Modal (notes variant controls width and lists same CSS custom properties). [2]
🌐 Web query:
PatternFly Flex component flexWrap responsive design
💡 Result:
Short answer — PatternFly supports responsive flex wrapping both in CSS classes and in the React Flex API.
-
CSS / utility approach: use the Flex layout classes and breakpoint modifiers (e.g. .pf-l-flex with modifier classes like .pf-m-column-on-lg, and related responsive modifiers) to control direction/wrapping at breakpoints. See the layout docs for examples and available breakpoint modifiers. [1]
-
React component approach: the Flex component exposes a flexWrap prop you can set responsively with breakpoint keys (default, sm, md, lg, xl, 2xl). Allowed values: 'wrap', 'wrapReverse', 'nowrap'. Example:
<Flex flexWrap={{ default: 'wrap', lg: 'nowrap' }}>...
This will apply wrap by default and switch to nowrap at the lg breakpoint. [2]
Sources:
[1] PatternFly Flex layout documentation (classes / responsive modifiers).
[2] PatternFly Flex component props (flexWrap API).
🌐 Web query:
PatternFly Split layout component narrow viewport handling
💡 Result:
Short answer — Split doesn't automatically stack at narrow widths; use wrapping or a different layout.
- PatternFly Split lays items horizontally and will shrink items unless you enable wrapping. Use the pf-m-wrap / isWrappable option on Split to allow items to wrap to new lines on narrow viewports. [1][2]
- Mark the item that should take remaining space with .pf-m-fill / isFilled so layout behaves predictably when space is constrained. [2]
- If you want a true stacked (vertical) behavior on small screens, use a layout/component that switches orientation (for example Sidebar uses a stack orientation on small screens by default), or switch between Split and Stack responsively in your markup/CSS. [3]
References
- PatternFly — Split (design guidelines). [1]
- PatternFly — Split (HTML / React props). [2]
- PatternFly — Sidebar (orientation: stack on small screens). [3]
Use PatternFly's responsive flexWrap API instead of inline style.
The flexWrap: 'nowrap' inline style prevents responsive behavior. PatternFly Flex component supports responsive flexWrap through breakpoint keys (e.g., flexWrap={{ default: 'wrap', lg: 'nowrap' }}), which would allow text to wrap on small screens and prevent wrapping only on larger viewports.
Change:
style={{ flexWrap: 'nowrap' }}To:
flexWrap={{ default: 'wrap', lg: 'nowrap' }}This ensures the modal content adapts properly on mobile devices while maintaining the horizontal layout on desktop screens.
🤖 Prompt for AI Agents
In frontend/public/components/modals/edit-yaml-settings-modal.tsx around lines
71 to 75, replace the inline style that forces non-wrapping with PatternFly's
responsive flexWrap prop so the layout can wrap on small screens and only stay
nowrap on large viewports; remove style={{ flexWrap: 'nowrap' }} and add
flexWrap={{ default: 'wrap', lg: 'nowrap' }} to the Flex component so text wraps
on mobile but remains horizontal on desktop.
|
/retitle OCPBUGS-65812: Layout issue on YAML editor settings popup window |
openshift-ci
bot
changed the title
openshift-ci-robot
added
jira/severity-low
|
@XiyunZhao: This pull request references Jira Issue OCPBUGS-65812, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
XiyunZhao
force-pushed
the
OCPBUGS-65812
branch
from
aa98e3f to
2bc6e8d
Compare
|
@XiyunZhao: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@logonoff thanks for the info, close the PR now |
|
@XiyunZhao: This pull request references Jira Issue OCPBUGS-65812. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In 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. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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 kubernetes-sigs/prow repository. |
5 participants
Before:
Now:
