-
Notifications
You must be signed in to change notification settings - Fork 2
Addons Page Template #334
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
base: staging
Are you sure you want to change the base?
Addons Page Template #334
Conversation
@ruz3n - Please solve the Storybook Tests issues that are failing in the pipeline. |
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.
Please check the Template Development Guideline and update the PR as per the document.
Also, please check the feedback points given below:
- The template does not match the Figma mockup.
- Verify and align the overall spacing, padding/margin, icon size, color, text size, and font weight with the Figma mockup.
icon: <Zap className="size-8" />, | ||
}, | ||
{ | ||
id: '3', |
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.
What: In the AddonCard component, the buttons for install and configure actions (onInstall and onConfigure) are just logging the actions. This may lead to an incomplete flow, risking failure in handling the installation/configuration process.
Why: By not implementing the logic to handle these actions, the buttons serve no functional purpose. This means the user interaction is not leading to any outcome and is misleading.
How: Implement the proper installation and configuration logic within those functions, or at least provide a stub for future implementation.
onConfigure?: ( addon: Addon ) => void; | ||
} | ||
|
||
const AddonCard = ( { addon, onInstall, onConfigure }: AddonCardProps ) => ( |
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.
What: There is usage of inline styles and class names for containers throughout the code. This might become inefficient and hard to maintain as the application grows, especially if styles need to change across multiple instances.
Why: Inline styles reduce the separation of concerns and can lead to redundancy, making it harder to manage in the long term and could impact performance due to increased style recalculations by the browser.
How: Consider using a styling module or global CSS for handling styles more effectively, grouping similar functions together, and making it simpler to change styles at a single point.
<Button variant="ghost" size="xs" className="p-1" aria-label="Support"> | ||
<HelpCircle className="size-4" /> | ||
</Button> | ||
</Container.Item> |
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.
What: Consider implementing proper error handling for the install and configure actions to ensure the user is informed about any issues that may arise while interacting with these features.
Why: Without error handling, users might face unexpected behaviors without any feedback, leading to frustration or confusion when something goes wrong during an interaction.
How: Use try-catch blocks for asynchronous operations or promise error handling strategies to catch and display user-friendly messages on the UI when errors occur.
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.
It seems previous feedback points are not resolved. Please resolve previous feedback points and request a re-review.
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.
Please watch the video and fix the issues mentioned in the video:
Description
This PR adds the design implementation corresponding to this figma link -> https://www.figma.com/design/5PxINaOCAK9NdEHMlQk382/Force-UI-Components?node-id=13545-43831&t=cvYnJuhmhOYOhpcv-4
The template shows the list of recommended plugins and themes
Screenshots
Types of changes
How has this been tested?
Checklist: