-
Notifications
You must be signed in to change notification settings - Fork 17
feat(Illustration): render with Icon component #2917
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
Conversation
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.
Pull Request Overview
This PR refactors the Illustration component to render using the Icon component instead of directly loading SVG images. The change improves type safety by using IconData and addresses rendering issues with sprite elements.
Key changes:
- Migrated from img-based rendering to Icon component rendering
- Changed width prop from string to number type for consistency
- Added proper TypeScript typing with IconData
Reviewed Changes
Copilot reviewed 8 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Illustration/Illustration.tsx | Complete refactor to use Icon component with IconData typing and async loading |
| src/components/Illustration/Illustration.scss | New CSS styles for Icon-based rendering with responsive sizing |
| config-overrides.js | Added illustrations directory to webpack SVG processing |
| src/containers/Tenants/Tenants.tsx | Updated width prop from string "200" to number 200 |
| src/containers/Tenant/Healthcheck/Healthcheck.tsx | Updated width prop from string "200" to number 200 |
| src/containers/Tenant/Diagnostics/Network/Network.tsx | Updated width prop from string "200" to number 200 |
| src/containers/Nodes/NodesTable.tsx | Updated width prop from string "200" to number 200 |
| src/components/EmptyFilter/EmptyFilter.tsx | Added explicit width={200} to default image prop |
7f63ce9 to
f19b7da
Compare
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.
Pull Request Overview
Copilot reviewed 8 out of 14 changed files in this pull request and generated 2 comments.
| &__icon { | ||
| width: 100%; | ||
| height: 100%; | ||
| } |
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.
Without these properties and explicitly set width and height, gravity-ui icons can use viewbox sizes. We need to be able to set only width or to use parent component sizes, so percentage sizes are needed here
https://github.com/gravity-ui/uikit/blob/main/src/components/Icon/Icon.tsx#L84
Closes #2239
Stand (AccessDenied should be displayed): https://nda.ya.ru/t/Ln81amqO7KK7a8
The goal is to fix icons rendering when embedded ui is used as a package
@svgr/webpackIconinstead ofimgheightandwidthin icons toviewboxCI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔽
Current: 85.36 MB | Main: 85.40 MB
Diff: 0.04 MB (-0.04%)
✅ Bundle size decreased.
ℹ️ CI Information