- Issue created by @narendraR
- Merge request !674Issue #3501036: Fixed wrong title of installed button on recipe → (Merged) created by Unnamed author
- 🇺🇸United States chrisfromredfin Portland, Maine
If you want to use a variant from a prop, you should change all of them and not just the 'installed' one. I think this is a good change, even if we don't capitalize on it immediately.
I would replace "package" with "project" as the default variant.
- First commit to issue fork.
- 🇮🇳India shalini_jha
I have addressed the feedback and made the necessary updates. The pipeline is green,
so I am moving this to 'Needs Review.' Kindly review. - 🇬🇪Georgia lekso surameli
Hey everyone, I'm going to review this MR as part of the ContributionWeekend2025
- 🇬🇪Georgia lekso surameli
Thank you all for your effort on this merge request
After reviewing the changes, I’ve concluded that the current approach isn’t entirely valid or functioning as intended.
The issue lies in the use of the variant prop. This prop is designed to control the display mode of the ProjectIcon component, but it’s currently being repurposed to represent the type of recipe. This approach doesn’t align with its intended purpose and leads to unintended side effects.
Additionally, the variant prop currently supports two values: project-listing and module-details, each with separate associated CSS classes for styling. Overloading this prop with additional responsibilities could interfere with its primary purpose and the related styles.
For example with these changes the tooltip for the icon looks like this:
Because the value of variant prop is module-details.
To address this, I recommend introducing a new prop specifically for controlling the type of recipe. This would make the component’s API more explicit and maintain the clarity and integrity of the existing variant prop.
Let me know if you’d like to discuss this further
I'm also changing the status of this ticket to Needs work
- First commit to issue fork.
- 🇺🇸United States phenaproxima Massachusetts
Didn't manually test but the code looks okay to me, and seems like a sensible change. The removal of the variant prop in favor of a generic term, at least for now, makes a ton of sense.
-
tim.plunkett →
committed 31756465 on 2.0.x authored by
nidhish →
Issue #3501036 by nidhish, shalini_jha, utkarsh_33, tim.plunkett, lekso...
-
tim.plunkett →
committed 31756465 on 2.0.x authored by
nidhish →
Automatically closed - issue fixed for 2 weeks with no activity.