-
Notifications
You must be signed in to change notification settings - Fork 2
BA-1850: Add Missing MDX Doc Files for BaseApp Frontend Packages - group 1 #167
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { Meta } from '@storybook/addon-docs' | ||
|
|
||
| <Meta title="@baseapp-frontend | designSystem/General/Logo" /> | ||
|
|
||
| # Component Documentation | ||
|
|
||
| ## Logo | ||
|
|
||
| - **Purpose**: A flexible component that wraps logo content in a consistent container with optional link functionality. | ||
| - **Expected Behavior**: Renders a container that centers its content. When not disabled, wraps the logo in a link to the homepage ("/"). | ||
|
|
||
| ## Use Cases | ||
|
|
||
| - **Current Usage**: | ||
| - Application header logos | ||
|
|
||
| ## Props | ||
|
|
||
| - **children** (ReactNode): The logo content to be displayed in the container | ||
| - **disabledLink** (boolean): If true, removes the link wrapper (defaults to false) | ||
| - **sx** (object): MUI system props for custom styling | ||
| - **...other**: Additional props are passed to the root Box component | ||
|
|
||
| ## Notes | ||
|
|
||
| - **Related Components**: | ||
| - Box: Used as the container component | ||
| - Link: Wraps the logo when clickable | ||
| - Various logo icon components that can be passed as children | ||
|
|
||
| ## Example Usage | ||
|
|
||
| ```javascript | ||
| import { Logo } from '@baseapp-frontend/design-system' | ||
| import { BaseAppLogoCondensed } from '@baseapp-frontend/design-system/icons' | ||
|
|
||
| const MyComponent = () => { | ||
| return ( | ||
| <Logo disabledLink={false}> | ||
| <BaseAppLogoCondensed /> | ||
| </Logo> | ||
| ) | ||
| } | ||
| export default MyComponent | ||
| ``` |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||||||||||||||||||||||||
| import { Meta } from '@storybook/addon-docs' | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <Meta title="@baseapp-frontend | designSystem/Popover/Popover" /> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Component Documentation | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ## Popover | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - **Purpose**: A customized wrapper around MUI's Popover component that provides consistent positioning and styling, with an optional arrow indicator. It's used for displaying floating content like menus and tooltips with precise positioning control. | ||||||||||||||||||||||||||||
| - **Expected Behavior**: Renders a popover anchored to a specified element (currently only AccountPopover), with configurable arrow placement and positioning. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ## Use Cases | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - **Current Usage**: | ||||||||||||||||||||||||||||
| - AccountPopover | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ## Props | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| - **open** (HTMLElement | null): The anchor element to position the popover against | ||||||||||||||||||||||||||||
| - **children** (ReactNode): Content to render inside the popover | ||||||||||||||||||||||||||||
| - **arrow** (string, optional): Position of the arrow indicator (default: 'top-right') | ||||||||||||||||||||||||||||
| - **hiddenArrow** (boolean, optional): Whether to hide the arrow indicator | ||||||||||||||||||||||||||||
| - **sx** (object, optional): Additional styles to apply to the popover | ||||||||||||||||||||||||||||
| - **...other**: All other props are passed to MUI Popover | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| - **open** (HTMLElement | null): The anchor element to position the popover against | |
| - **children** (ReactNode): Content to render inside the popover | |
| - **arrow** (string, optional): Position of the arrow indicator (default: 'top-right') | |
| - **hiddenArrow** (boolean, optional): Whether to hide the arrow indicator | |
| - **sx** (object, optional): Additional styles to apply to the popover | |
| - **...other**: All other props are passed to MUI Popover | |
| - **open** (boolean): Whether the popover is currently open | |
| - **anchorEl** (HTMLElement | null): The anchor element to position the popover against | |
| - **children** (ReactNode): Content to render inside the popover | |
| - **arrow** (string, optional): Position of the arrow indicator (default: 'top-right') | |
| - **hiddenArrow** (boolean, optional): Whether to hide the arrow indicator | |
| - **sx** (object, optional): Additional styles to apply to the popover | |
| - **...other**: All other props are passed to MUI Popover |
Outdated
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.
🛠️ Refactor suggestion
Enhance example usage with trigger element
The example is incomplete as it doesn't show the trigger element that opens the popover.
```javascript
import { Popover } from '@baseapp-frontend/design-system'
+import { Button } from '@mui/material'
const MyComponent = () => {
const [anchorEl, setAnchorEl] = useState<HTMLElement | null>(null)
const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => {
setAnchorEl(event.currentTarget)
}
const handleClose = () => {
setAnchorEl(null)
}
return (
+ <>
+ <Button onClick={handleClick}>
+ Open Popover
+ </Button>
<Popover open={anchorEl} onClose={handleClose}>
<div>This is the content of the Popover</div>
</Popover>
+ </>
)
}
export default MyComponent
<!-- This is an auto-generated comment by CodeRabbit -->
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,53 @@ | ||||||||||||||
| import { Meta } from '@storybook/addon-docs' | ||||||||||||||
|
|
||||||||||||||
| <Meta title="@baseapp-frontend | designSystem/Avatars/AvatarWithPlaceholder" /> | ||||||||||||||
|
||||||||||||||
| import { Meta } from '@storybook/addon-docs' | |
| <Meta title="@baseapp-frontend | designSystem/Avatars/AvatarWithPlaceholder" /> | |
| import { Meta } from '@storybook/addon-docs' | |
| <Meta title="@baseapp-frontend-packages | designSystem/Avatars/AvatarWithPlaceholder" /> |
Outdated
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.
🛠️ Refactor suggestion
Enhance placeholder behavior documentation
The documentation should specify:
- What the default placeholder looks like
- How the placeholder is generated (e.g., initials from text)
- Customization options for the placeholder
Outdated
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.
🛠️ Refactor suggestion
Add package-specific use cases
The current use cases are too generic. As mentioned in the PR comments, please provide specific examples of where this component is used within the BaseApp packages.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||||||||||||||||||||||||||||||
| import { Meta } from '@storybook/addon-docs' | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| <Meta title="@baseapp-frontend | designSystem/Avatars/ClickableAvatar" /> | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| import { Meta } from '@storybook/addon-docs' | |
| <Meta title="@baseapp-frontend | designSystem/Avatars/ClickableAvatar" /> | |
| import { Meta } from '@storybook/addon-docs' | |
| <Meta title="@baseapp-frontend-packages | designSystem/Avatars/ClickableAvatar" /> |
Outdated
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.
Replace hardcoded image URL with a placeholder
The example uses a hardcoded DigitalOcean Spaces URL which may not be accessible and could become invalid. Replace it with a placeholder or local asset path.
<Image
- src="https://nyc3.digitaloceanspaces.com/baseapp-production-storage/media/user-avatars/5/6/4/resized/50/50/185a04dfdaa512d218cf9b7a5097e3c9.png"
+ src="/assets/placeholder-avatar.png"
alt="Avatar Icon"
width={50}
height={50}
/>📝 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.
| <ClickableAvatar isOpen={false} width={50} height={50} onClick={() => {}}> | |
| <Image | |
| src="https://nyc3.digitaloceanspaces.com/baseapp-production-storage/media/user-avatars/5/6/4/resized/50/50/185a04dfdaa512d218cf9b7a5097e3c9.png" | |
| alt="Avatar Icon" | |
| width={50} | |
| height={50} | |
| /> | |
| </ClickableAvatar> | |
| <ClickableAvatar isOpen={false} width={50} height={50} onClick={() => {}}> | |
| <Image | |
| src="/assets/placeholder-avatar.png" | |
| alt="Avatar Icon" | |
| width={50} | |
| height={50} | |
| /> | |
| </ClickableAvatar> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { Meta } from '@storybook/addon-docs' | ||
|
|
||
| <Meta title="@baseapp-frontend | designSystem/Buttons/IconButton" /> | ||
|
|
||
| # Component Documentation | ||
|
|
||
| ## IconButton | ||
|
|
||
| - **Purpose**: The IconButton component provides a clickable button that displays an icon. | ||
| - **Expected Behavior**: The IconButton renders as a circular button containing an icon. It responds to hover and click states with visual feedback, and can be disabled when needed. | ||
|
|
||
| ## Use Cases | ||
|
|
||
| - **Current Usage**: | ||
| - Navigation menus | ||
| - Toolbars | ||
| - Action buttons in tables/lists | ||
| - Close buttons for modals/dialogs | ||
| - Toggle buttons for expandable content | ||
| - **Potential Usage**: | ||
| - Social media sharing buttons | ||
| - Media player controls | ||
| - Form field actions (clear, show/hide password) | ||
| - Mini floating action buttons | ||
|
|
||
| ## Props | ||
|
|
||
| - **children** (ReactNode): The icon element to be displayed inside the button. | ||
| - **color** (string): The color of the button. | ||
| - **disabled** (boolean): If true, the button will be disabled. | ||
| - **isLoading** (boolean): If true, the button will be in a loading state. | ||
| - **onClick** (function): The function to be called when the button is clicked. | ||
| - **hasTooltip** (boolean): If true, the button will have a tooltip. | ||
|
|
||
|
||
| ## Notes | ||
|
|
||
| - **Related Components**: | ||
| - Button: Standard button component for text-based actions | ||
| - Tooltip: Often wraps IconButton to provide additional context | ||
| - Menu: Can be triggered by IconButton | ||
| - Icons: Provides the visual elements used within IconButton | ||
|
|
||
| ## Example Usage | ||
|
|
||
| ```javascript | ||
| import { IconButton } from '@baseapp-frontend/design-system' | ||
| import { CloseIcon } from '@baseapp-frontend/design-system/icons' | ||
|
|
||
| const MyComponent = () => { | ||
| return ( | ||
| <IconButton | ||
| hasTooltip={true} | ||
| isLoading={false} | ||
| disabled={false} | ||
| color="primary" | ||
| onClick={() => {}} | ||
| > | ||
| <CloseIcon /> | ||
| </IconButton> | ||
| ) | ||
| } | ||
| export default MyComponent | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||||||||||
| import { Meta } from '@storybook/addon-docs' | ||||||||||||||
|
|
||||||||||||||
| <Meta title="@baseapp-frontend | designSystem/Dialogs/ConfirmDialog" /> | ||||||||||||||
|
||||||||||||||
| import { Meta } from '@storybook/addon-docs' | |
| <Meta title="@baseapp-frontend | designSystem/Dialogs/ConfirmDialog" /> | |
| import { Meta } from '@storybook/addon-docs' | |
| <Meta title="designSystem/Dialogs/ConfirmDialog" /> |
Outdated
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.
🛠️ Refactor suggestion
Add BaseApp-specific implementation details
While the general purpose and behavior are well described, please add specific details about how this component is used within BaseApp. For example:
- What are the standard styling conventions?
- Are there any BaseApp-specific behaviors or configurations?
- What are the accessibility considerations in the BaseApp implementation?
Outdated
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.
🛠️ Refactor suggestion
Replace generic use cases with actual BaseApp implementations
Instead of listing generic use cases, document specific implementations within BaseApp. For each use case:
- Reference the actual component/page where it's being used
- Describe the specific confirmation scenarios
- Include any custom configurations used
Outdated
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.
🛠️ Refactor suggestion
Enhance props documentation with accessibility and customization options
The props documentation should include:
- Accessibility props (e.g.,
aria-labelledby,aria-describedby) - Styling/theming props (e.g.,
className,style) - Size/variant props if supported
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.
Version and changelog details need improvement
The major version bump (1.0.0) seems inconsistent with the nature of changes:
The changelog entry lacks specific details:
Consider:
📝 Committable suggestion