-
Notifications
You must be signed in to change notification settings - Fork 4
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
[WEB-3979] Encapsulated icons #487
Conversation
Warning Rate limit exceeded@jamiehenson has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 56 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new React component, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/core/Icon/EncapsulatedIcon.tsx (1)
1-33
: LGTM!The
EncapsulatedIcon
component is well-structured and provides a nice way to render icons with a customizable appearance. The use of default prop values and thet
function for determining theme colors is a good approach.A few suggestions for further improvement:
Consider adding prop types validation using
PropTypes
or TypeScript to ensure the correct types are passed to the component.The component could be made more reusable by allowing the user to pass additional CSS classes or styles as props, which can be merged with the existing classes and styles.
The magic numbers used for padding and size calculations (e.g.,
size - 2
,size - 12
) could be extracted into named constants to improve readability and maintainability.Overall, the component is a great addition to the codebase and enhances the UI by providing a visually distinct and thematically consistent representation of icons.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (7)
src/core/Icon/__snapshots__/Icon.stories.tsx.snap
is excluded by!**/*.snap
src/core/ProductTile/__snapshots__/ProductTile.stories.tsx.snap
is excluded by!**/*.snap
src/core/icons/icon-product-asset-tracking-encapsulated.svg
is excluded by!**/*.svg
src/core/icons/icon-product-chat-encapsulated.svg
is excluded by!**/*.svg
src/core/icons/icon-product-livesync-encapsulated.svg
is excluded by!**/*.svg
src/core/icons/icon-product-pubsub-encapsulated.svg
is excluded by!**/*.svg
src/core/icons/icon-product-spaces-encapsulated.svg
is excluded by!**/*.svg
Files selected for processing (5)
- src/core/Icon/EncapsulatedIcon.tsx (1 hunks)
- src/core/Icon/Icon.stories.tsx (4 hunks)
- src/core/Icon/types.ts (0 hunks)
- src/core/ProductTile.tsx (2 hunks)
- src/core/ProductTile/data.ts (1 hunks)
Files not reviewed due to no reviewable changes (1)
- src/core/Icon/types.ts
Files skipped from review due to trivial changes (1)
- src/core/ProductTile/data.ts
Additional comments not posted (15)
src/core/ProductTile.tsx (10)
2-2
: LGTM!The import statement for the new
EncapsulatedIcon
component is correct and aligns with the changes mentioned in the summary.
5-5
: LGTM!The import statement for the
ColorClass
type is correct and aligns with the changes mentioned in the summary.
6-6
: LGTM!The import statement for the
determineThemeColor
function is correct and aligns with the changes mentioned in the summary.
25-26
: LGTM!The new function
t
correctly encapsulates the logic for determining the theme color based on theselected
state and the providedcolor
class using thedetermineThemeColor
function.
30-30
: LGTM!The usage of the
t
function to dynamically determine the background color class based on theselected
state is correct and aligns with the changes mentioned in the summary.
33-36
: LGTM!The usage of the
EncapsulatedIcon
component with thename
andtheme
props is correct and aligns with the changes mentioned in the summary.
41-41
: LGTM!The usage of the
t
function to dynamically determine the text color class based on theselected
state is correct and aligns with the changes mentioned in the summary.
46-46
: LGTM!The usage of the
t
function to dynamically determine the text color class based on theselected
state is correct and aligns with the changes mentioned in the summary.
54-54
: LGTM!The usage of the
text-gui-unavailable
class for the "COMING SOON" status is correct and aligns with the changes mentioned in the summary, improving visual consistency.
60-66
: LGTM!The usage of the
t
function to dynamically determine the text color class based on theselected
state is correct and aligns with the changes mentioned in the summary.src/core/Icon/Icon.stories.tsx (5)
3-3
: LGTM!The import statement for
EncapsulatedIcon
is necessary and the import path is correct.
6-6
: LGTM!The import statement for
Theme
type is necessary and the import path is correct.
17-21
: LGTM!The changes to the
renderIcons
function signature are valid and backward compatible. The new optional parameters,encapsulated
andtheme
, allow for flexibility in rendering encapsulated icons with theme variations.
34-43
: LGTM!The conditional rendering logic is implemented correctly to switch between the
EncapsulatedIcon
and the standardIcon
based on theencapsulated
flag. Thetheme
prop is properly passed to theEncapsulatedIcon
with a default value of "dark". The existingIcon
component usage remains unaffected.
102-104
: LGTM!The new story exports,
DarkEncapsulatedIcons
andLightEncapsulatedIcons
, are implemented correctly. They effectively demonstrate the usage of theEncapsulatedIcon
component with different theme variations. These stories enhance the documentation and provide visual examples of the encapsulated icons in both dark and light themes.Also applies to: 106-108
3accf51
to
d3f3c70
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.
As 🐰🤖 said LGTM!
252a6ae
to
857d70b
Compare
857d70b
to
2ef7b82
Compare
Summary of changes
Not sure if Coderabbit will provide it's own summary anyway, but this PR adds theming to
ProductTile
, and allows icons to be dynamically encapsulated instead of providing separate assets (also useful with theming).How do you manually test this?
Storybook link (at the bottom, the anchor link doesnt work)
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
EncapsulatedIcon
component for styled icon rendering with customizable themes and sizes.Bug Fixes
Chores