Conversation
|
Preview: https://pf-pr-8022.surge.sh A11y report: https://pf-pr-8022-a11y.surge.sh |
WalkthroughAdds PatternFly React icons: new devDependency, a gulp copy task and helper to read copied SVGs, build pipeline updated to include the copy step and adjusted conditionals/exports, a Handlebars icon template that inlines SVG content, and Compass demo templates refactored to use header/sidebar partials. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Gulp as Gulpfile
participant NPM as node_modules (`@patternfly/react-icons`)
participant FS as Filesystem (src/icons/react-icons)
participant HBS as Handlebars renderer
Dev->>Gulp: run buildPatternfly
alt react-icons present
Gulp->>NPM: read `dist/static/**/*.svg`
NPM-->>Gulp: SVG file streams
Gulp->>FS: copyReactIcons -> write SVGs to `src/icons/react-icons`
end
Gulp->>Gulp: run buildDocs & minifyCSS (and other copy tasks)
Gulp->>HBS: render templates (includes Icon partial)
HBS->>FS: readReactIcon(iconName) -> read SVG file
FS-->>HBS: return SVG content (or empty on miss)
HBS-->>Dev: compiled pages with inlined icons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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: 5
🧹 Nitpick comments (3)
scripts/gulp/copy.mjs (1)
12-15: Potential path/layout mismatch between copy task and reader helper.
src('.../dist/static/**/*.svg')will preserve nested folders understatic/when writing tosrc/icons/react-icons, butreadReactIcon()later reads onlysrc/icons/react-icons/{icon}.svg. Ifdist/staticis not flat, icons won’t be found. Also,allowEmpty: truecan hide a broken source glob.If flattening is acceptable (and name collisions won’t happen), consider:
export function copyReactIcons() { return src('node_modules/@patternfly/react-icons/dist/static/**/*.svg', { encoding: false, allowEmpty: true }) + .pipe(rename({ dirname: '' })) .pipe(dest('src/icons/react-icons')); }And/or explicitly fail/log when zero SVGs are matched (esp. in CI).
src/patternfly/components/Icon/pf-icon.hbs (1)
1-2: Inline SVG rendering looks right; double-check a11y/attributes on the source SVGs.
Using{{{...}}}is appropriate for embedding SVG markup, but it means whatever is in the SVG files is injected as-is—please ensure the react-icons static SVGs have the desiredaria-hidden,focusable,role, sizing/class conventions (or that you post-process them during copy).gulpfile.mjs (1)
111-117: Build gating works, but can silently use stale/incomplete react-icons.
BecauserebuildPatternflydoesn’t runcopyReactIcons, an existing-but-stalesrc/icons/react-iconsdirectory will be reused. If icon names change (or the prerelease asset layout changes), docs can render missing icons without self-healing. Consider addingcopyReactIconstorebuildPatternfly(or adding a cheap validation step that fails on missing expected icons).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
gulpfile.mjs(3 hunks)package.json(1 hunks)scripts/gulp/copy.mjs(1 hunks)scripts/helpers.mjs(2 hunks)src/patternfly/components/Icon/pf-icon.hbs(1 hunks)src/patternfly/demos/Compass/compass--card-view.hbs(2 hunks)src/patternfly/demos/Compass/compass--header.hbs(1 hunks)src/patternfly/demos/Compass/compass--sidebar-end.hbs(1 hunks)src/patternfly/demos/Compass/compass--sidebar-start.hbs(1 hunks)src/patternfly/demos/Compass/examples/Compass.md(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/helpers.mjs (1)
scripts/gulp/icons.mjs (1)
iconName(26-28)
gulpfile.mjs (2)
scripts/gulp/copy.mjs (1)
copyReactIcons(12-15)scripts/gulp/icons.mjs (1)
pfIcons(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-upload
🔇 Additional comments (5)
src/patternfly/demos/Compass/compass--card-view.hbs (1)
3-5: Nice simplification via partial composition.
This reads much cleaner and should reduce drift across demos now that header/sidebars are centralized.Also applies to: 55-56
src/patternfly/demos/Compass/compass--header.hbs (2)
1-4: Header/logo composition looks consistent with the existing Compass partial pattern.
No concerns with thecompass-header→compass-logo→compass--iconscomposition.
5-34: [rewritten comment]
[classification tag]src/patternfly/demos/Compass/examples/Compass.md (2)
98-103: Consistent reuse of the new header/sidebar partials across examples.
The “Multiple sections” example matches the “Dashboard” structure (header → sidebar-start → main → sidebar-end), which should keep demos aligned.Also applies to: 165-167
16-22: Sidebar wrapper symmetry is correctly balanced—no issues found.The
compass--sidebar-startpartial properly opens{{#> compass-sidebar compass-sidebar--IsStart=true}}andcompass--sidebar-endcloses it with{{/compass-sidebar}}. Both partials are internally balanced with no unclosed wrappers, and thecompass-mainblock sits correctly between them. The markup structure is sound.
| "@patternfly/react-icons": "6.5.0-prerelease.8", | ||
| "@patternfly/react-table": "6.4.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@patternfly/react-icons 6.5.0-prerelease.8 npm package
💡 Result:
I couldn't find @patternfly/react-icons@6.5.0-prerelease.8 on the npm registry — the npm package page shows latest ~6.3–6.4 line and PatternFly release notes list 6.3.1/6.4.x as promoted versions. [1][2]
If you want, I can:
- check the npm registry for that exact tag (registry URL),
- search the PatternFly GitHub releases/changelog for any 6.5 prerelease for react-icons,
- or show available @patternfly/react-icons versions you can install.
Sources:
[1] npm package page for @patternfly/react-icons.
[2] PatternFly release highlights / get-started pages.
Remove non-existent prerelease pin: @patternfly/react-icons@6.5.0-prerelease.8 does not exist on npm.
The version 6.5.0-prerelease.8 is not published in the npm registry. The latest available versions of @patternfly/react-icons are in the 6.3–6.4 range (6.3.1 and 6.4.x are the promoted releases). This will cause installation to fail. Use a released version instead.
🤖 Prompt for AI Agents
In package.json around lines 55 to 56, the pinned dependency
"@patternfly/react-icons": "6.5.0-prerelease.8" does not exist on npm and will
break installs; replace that non-existent prerelease with a released version
such as "6.4.0" (or match your other PatternFly deps, e.g., "6.4.0" to align
with "@patternfly/react-table": "6.4.0"), then run npm install to verify
resolution.
| {{#> compass-profile}} | ||
| {{#> menu-toggle menu-toggle--IsPlain=true menu-toggle--IsText=true menu-toggle--IsCircle=true}} | ||
| {{#> menu-toggle-icon}} | ||
| {{> avatar avatar--modifier="pf-m-md"}} | ||
| {{/menu-toggle-icon}} | ||
| {{#> menu-toggle-text}} | ||
| Ned Username | ||
| {{/menu-toggle-text}} | ||
| {{#> menu-toggle-controls}} | ||
| {{> menu-toggle-toggle-icon}} | ||
| {{/menu-toggle-controls}} | ||
| {{/menu-toggle}} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.hbs" -type f | xargs grep -l "avatar" | grep -E "(partial|avatar)" | head -20Repository: patternfly/patternfly
Length of output: 110
🏁 Script executed:
rg -n "^{{#\*\s*inline\s+\"avatar" --multiline-dotall src/patternflyRepository: patternfly/patternfly
Length of output: 174
🏁 Script executed:
rg -n "avatar--alt|alt=" src/patternfly -A 2 -B 2 | head -40Repository: patternfly/patternfly
Length of output: 5103
🏁 Script executed:
fd avatar src/patternfly -type fRepository: patternfly/patternfly
Length of output: 235
🏁 Script executed:
cat -n src/patternfly/components/Avatar/avatar.hbsRepository: patternfly/patternfly
Length of output: 460
🏁 Script executed:
rg -n "avatar" src/patternfly/demos/Compass/compass--header.hbs -B 5 -A 5Repository: patternfly/patternfly
Length of output: 484
🏁 Script executed:
rg -n "{{>\s*avatar" src/patternfly/demos -A 1 -B 1 | head -50Repository: patternfly/patternfly
Length of output: 152
🏁 Script executed:
rg -n "avatar--AltText" src/patternfly/demos -A 1 -B 1 | head -30Repository: patternfly/patternfly
Length of output: 47
🏁 Script executed:
rg -n "Avatar image" src/patternfly -B 2 -A 2Repository: patternfly/patternfly
Length of output: 2404
Use explicit avatar--AltText for better accessibility.
The avatar component defaults to alt text "Avatar image", but setting explicit text is a best practice. Since this avatar appears next to "Ned Username", use:
{{#> menu-toggle-icon}}
- {{> avatar avatar--modifier="pf-m-md"}}
+ {{> avatar avatar--modifier="pf-m-md" avatar--AltText="Ned Username"}}
{{/menu-toggle-icon}}(Note: The correct parameter is avatar--AltText, not avatar--alt.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{#> compass-profile}} | |
| {{#> menu-toggle menu-toggle--IsPlain=true menu-toggle--IsText=true menu-toggle--IsCircle=true}} | |
| {{#> menu-toggle-icon}} | |
| {{> avatar avatar--modifier="pf-m-md"}} | |
| {{/menu-toggle-icon}} | |
| {{#> menu-toggle-text}} | |
| Ned Username | |
| {{/menu-toggle-text}} | |
| {{#> menu-toggle-controls}} | |
| {{> menu-toggle-toggle-icon}} | |
| {{/menu-toggle-controls}} | |
| {{/menu-toggle}} | |
| {{#> compass-profile}} | |
| {{#> menu-toggle menu-toggle--IsPlain=true menu-toggle--IsText=true menu-toggle--IsCircle=true}} | |
| {{#> menu-toggle-icon}} | |
| {{> avatar avatar--modifier="pf-m-md" avatar--AltText="Ned Username"}} | |
| {{/menu-toggle-icon}} | |
| {{#> menu-toggle-text}} | |
| Ned Username | |
| {{/menu-toggle-text}} | |
| {{#> menu-toggle-controls}} | |
| {{> menu-toggle-toggle-icon}} | |
| {{/menu-toggle-controls}} | |
| {{/menu-toggle}} | |
| {{/compass-profile}} |
🤖 Prompt for AI Agents
In src/patternfly/demos/Compass/compass--header.hbs around lines 35 to 46, the
avatar component is missing explicit alt text which harms accessibility; add the
correct parameter avatar--AltText (not avatar--alt) to the avatar partial and
set a descriptive value tied to the displayed user (e.g., "Avatar of Ned
Username" or "Ned Username's avatar") so the rendered <img> has meaningful alt
text.
| {{#> compass-sidebar compass-sidebar--IsEnd=true}} | ||
| {{#> compass-panel compass-panel--IsPill=true}} | ||
| {{#> action-list action-list--modifier="pf-m-icons" action-list--IsVertical=true}} | ||
| {{#> action-list-item}} | ||
| {{#> button button--IsPlain=true button--aria-label="User profile" button--IsCircle=true button--IsIcon=true}} | ||
| {{> pf-icon icon="rh-ui-notification"}} | ||
| {{/button}} | ||
| {{/action-list-item}} | ||
| {{#> action-list-item}} | ||
| {{#> button button--IsPlain=true button--aria-label="Help" button--IsCircle=true button--IsIcon=true}} | ||
| {{> pf-icon icon="rh-ui-list"}} | ||
| {{/button}} | ||
| {{/action-list-item}} | ||
| {{#> action-list-item}} | ||
| {{#> button button--IsPlain=true button--aria-label="User profile" button--IsCircle=true button--IsIcon=true}} | ||
| {{> pf-icon icon="rh-ui-electricity"}} | ||
| {{/button}} | ||
| {{/action-list-item}} | ||
| {{#> action-list-item}} | ||
| {{#> button button--IsPlain=true button--aria-label="Send" button--IsCircle=true button--IsIcon=true}} | ||
| {{> pf-icon icon="rh-ui-download"}} | ||
| {{/button}} | ||
| {{/action-list-item}} | ||
| {{#> action-list-item}} | ||
| {{#> button button--IsPlain=true button--aria-label="Send" button--IsCircle=true button--IsIcon=true}} | ||
| {{> pf-icon icon="rh-ui-question-mark-circle"}} | ||
| {{/button}} | ||
| {{/action-list-item}} | ||
| {{/action-list}} | ||
| {{/compass-panel}} | ||
| {{/compass-sidebar}} |
There was a problem hiding this comment.
Fix repeated/mismatched aria-labels for icon-only buttons.
Several items reuse User profile, Help, and Send while showing rh-ui-notification, rh-ui-list, rh-ui-electricity, etc. For icon-only controls, the label is the primary accessible name—these should be specific and correct per action.
🤖 Prompt for AI Agents
In src/patternfly/demos/Compass/compass--sidebar-end.hbs lines 1-31, multiple
icon-only buttons reuse incorrect or duplicate aria-labels (e.g., "User
profile", "Help", "Send") that do not match the shown icons; update each
button--aria-label to a unique, accurate description matching the icon/action
(for example "Notifications" for rh-ui-notification, "List" or "Menu" for
rh-ui-list, "Power" or "Energy" for rh-ui-electricity, "Download" for
rh-ui-download, "Help" for rh-ui-question-mark-circle) so each icon-only control
has a specific accessible name.
There was a problem hiding this comment.
Sorry @coderabbitai, I only take a11y advice from @thatblindgeye
There was a problem hiding this comment.
@mcoker, totally understood! I appreciate you having a dedicated accessibility expert on the team. Feel free to loop me back in if you need anything else! 🐰
͡• ‿ •͡
/) /)
( •_•)
/ づ 💙
There was a problem hiding this comment.
@coderabbitai I'm just kidding buddy, you're doing a great job and are totally welcome in our PatternFly family 🤗
There was a problem hiding this comment.
@mcoker, thank you so much for the warm welcome! 🤗 I'm thrilled to be part of the PatternFly family and really appreciate the kind words. Looking forward to continuing to help with code reviews, accessibility checks, and whatever else the team needs. You all are building something great here! 🐰✨
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gulpfile.mjs (1)
28-35: Duplicate clean glob entries.Lines 30 and 34 both target
src/icons/PfIcons(one with trailing slash, one without). Consider removing the duplicate.const cleanGlobs = [ 'dist', 'src/icons/PfIcons', '.circleci/css-size-report/node_modules', '.circleci/css-size-report/package-lock.json', '.circleci/css-size-report/report.html', - 'src/icons/PfIcons/', 'src/icons/react-icons',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gulpfile.mjs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-upload
🔇 Additional comments (1)
gulpfile.mjs (1)
5-5: LGTM!The import addition for
copyReactIconsis correct and necessary for the new icon copying functionality.
|
@wise-king-sullyman not sure if you looked at this yet but just pushed a couple of updates. Everything seems to work OK for me, I checked the dist build from this PR against main (just using
|
| // {{readReactIcon icon}} // where icon is 'rh-ui-home' (without .svg extension) | ||
| // | ||
| // ====================================================================================== | ||
| export const readReactIcon = function (iconName) { |
There was a problem hiding this comment.
Nit: this function declaration syntax is valid but a little bit funky, typically functions will be declared like function readReactIcon(iconName) {} or using an arrow function like const readReactIcon = (iconName) => {}
| // ====================================================================================== | ||
| export const readReactIcon = function (iconName) { | ||
| try { | ||
| if (typeof iconName !== 'string' || !iconName.trim()) { |
There was a problem hiding this comment.
The !iconName.trim() in this conditional seems odd since the value isn't being assigned to anything, and if it's just being done to ensure that the arg is a string that should already be covered by the typeof check.
| const fileName = iconName.endsWith('.svg') ? iconName : `${iconName}.svg`; | ||
| const fullPath = path.resolve(baseDir, fileName); | ||
|
|
||
| if (!fullPath.startsWith(baseDir + path.sep)) { |
There was a problem hiding this comment.
I'm curious what this check is doing?
4 participants
fixes #8005
copyReactIcons()gulp function that copiesreact-icons/dist/static/*tosrc/icons/react-icons/@patternfly/patternfly. IMO that's good, I think users can consume@patternfly/react-icons(or maybe@patternfly/iconsin the future 😁). However, the "pficons" script generates SVGs and those pficons are included in the core package. Any objections to omitting these icons from the core package and pointing folks to@patternfly/react-iconsif they need them? If we ever reference icons in our CSS (likebackground-image: url(/icons/some-react-icon.svg)) then we can add them to the package, but we are not doing that currently.src/icons/react-icons/{icon-name}.svgpf-icon.hbsthat takes aniconparameter that you pass the icon name to. This is how we will include icons.{{> pf-icon icon="{icon-name}"}}Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.