chore: add storybook integration for footer #5
Conversation
Signed-off-by: Asitha de Silva <[email protected]>
WalkthroughThis pull request introduces several configuration files and components for integrating Storybook into the project. It includes the creation of Changes
Possibly related PRs
Poem
Warning Rate limit exceeded@asithade has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 32 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
package.json (1)
15-15: Consider making the Storybook port configurableThe Storybook port is hardcoded to 6006. Consider making it configurable through an environment variable to avoid potential port conflicts.
- "storybook": "storybook dev -p 6006", + "storybook": "storybook dev -p ${PORT:-6006}",docs/footer.md (1)
52-62: Enhance the development section with additional details.While the basic instructions are clear, consider adding the following information to make the documentation more comprehensive:
# Development -To develop the component, run the following commands: +## Prerequisites + +Ensure you have all dependencies installed: + +```bash +npm install +``` + +## Running the Development Environment + +To develop the component: ```bash npm run build npm run storybook-This will build the component and start the storybook server. You can then navigate to http://localhost:6006 to view the component. Updates made to the component will be reflected live in the storybook environment.
+This will:
+1. Build the component
+2. Start the Storybook server at http://localhost:6006
+
+## Available Stories
+
+In Storybook, you can:
+- View the footer component in different states
+- Interact with the component's properties
+- View the component's documentation
+
+## Troubleshooting
+
+If you encounter issues:
+- Ensure all dependencies are installed
+- Clear the build cache:npm run clean
+- Check the console for error messages</blockquote></details> <details> <summary>src/components/footer/footer.component.ts (7)</summary><blockquote> `4-15`: **Consider enhancing documentation with usage examples and accessibility information.** The JSDoc comments are well-structured, but could be improved by adding: - Example usage of the component - ARIA attributes and accessibility considerations - Browser compatibility information ```diff /** * @element lfx-footer * @summary A footer component for LFX applications * @description This component provides a consistent footer across LFX applications + * + * @example + * ```html + * <lfx-footer></lfx-footer> + * ``` + * * @csspart footer-container - The main container of the footer * @csspart footer-content - The content wrapper of the footer * @csspart footer-links - The container for footer links * @csspart copyright-container - The container for copyright information * * @cssproperty --lfx-footer-bg - Background color of the footer * @cssproperty --lfx-footer-text - Text color of the footer + * + * @accessibility + * The footer component should be wrapped in a <footer> element with role="contentinfo" + * Links should have appropriate focus indicators and hover states */
Line range hint
16-20: Add browser support detection for Shadow DOM.Consider adding a fallback mechanism for browsers that don't support Shadow DOM.
constructor() { super(); + if (!('attachShadow' in Element.prototype)) { + console.warn('Shadow DOM is not supported in this browser. Some styles may not work as expected.'); + return; + } this.attachShadow({ mode: 'open' }); }
25-26: Consider using font-display and preload for better font loading performance.The Google Fonts import might cause FOUT (Flash of Unstyled Text). Consider using
font-display: swapand preloading the font.Add this to your document's
<head>:<link rel="preconnect" href="https://fonts.googleapis.com"> <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> <link rel="preload" as="style" href="https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap">
Line range hint
52-55: Fix missing semicolon in CSS rule and convert colors to custom properties.There's a syntax error in the CSS and the colors should be customizable.
.footer-links { display: flex; gap: 1rem; - color: #808b91 + color: #808b91; font-size: 0.75rem; }Also, consider converting hard-coded colors to CSS custom properties:
:host { --lfx-footer-bg: transparent; --lfx-footer-text: #5b6367; + --lfx-footer-link: #808b91; + --lfx-footer-link-hover: #5b6367; }
Line range hint
57-66: Add focus styles for accessibility.Links should have visible focus indicators for keyboard navigation.
.footer-links a { - color: #5b6367; + color: var(--lfx-footer-link); text-decoration: none; font-size: 0.75rem; + outline: none; } .footer-links a:hover { text-decoration: underline; - color: #5b6367; + color: var(--lfx-footer-link-hover); } + +.footer-links a:focus-visible { + outline: 2px solid var(--lfx-footer-link-hover); + outline-offset: 2px; +}
Line range hint
108-112: Enhance lifecycle method implementation.Consider adding error handling and cleanup.
connectedCallback() { + if (!this.shadowRoot) { + console.error('Shadow root is not available'); + return; + } - if (this.shadowRoot) { - this.shadowRoot.innerHTML = this.getTemplate(); - } + this.shadowRoot.innerHTML = this.getTemplate(); +} + +disconnectedCallback() { + // Clean up any event listeners or resources if needed }
Line range hint
114-120: Consider simplifying component registration.The IIFE pattern could be simplified, and error handling could be added.
-(() => { - if (typeof window !== 'undefined') { - if (!customElements.get('lfx-footer')) { - customElements.define('lfx-footer', LFXFooter); - } +if (typeof window !== 'undefined') { + try { + if (!customElements.get('lfx-footer')) { + customElements.define('lfx-footer', LFXFooter); + } + } catch (error) { + console.error('Failed to register lfx-footer component:', error); } -})(); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.storybook/main.ts(1 hunks).storybook/preview.ts(1 hunks)README.md(1 hunks)docs/footer.md(1 hunks)package.json(1 hunks)src/components/footer/browser.ts(0 hunks)src/components/footer/footer.component.ts(2 hunks)src/components/footer/footer.stories.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/footer/browser.ts
✅ Files skipped from review due to trivial changes (4)
- .storybook/main.ts
- .storybook/preview.ts
- README.md
- src/components/footer/footer.stories.ts
🔇 Additional comments (1)
docs/footer.md (1)
Line range hint 1-51: LGTM! Existing documentation is clear and well-structured.
The implementation guides for Angular, Vue, and VanillaJS are comprehensive and include working examples.
Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
src/components/footer/footer.style.ts (2)
21-36: Consider adding responsive designThe footer layout could benefit from responsive design considerations for different screen sizes.
Consider adding media queries:
.footer-container { margin: 0 auto; display: flex; justify-content: center; align-items: center; + max-width: 1200px; + width: 100%; + padding: 0 1rem; } .footer-content { text-align: center; font-size: 0.75rem; display: flex; justify-content: center; align-items: center; flex-direction: column; gap: 1rem; + width: 100%; }
61-72: Consolidate repeated styles using CSS custom propertiesCommon styles for colors and interactions are duplicated across different selectors.
Consider consolidating styles:
:host { /* ... existing properties ... */ + --footer-link-color: #5b6367; + --footer-text-size: 0.75rem; } +/* Common link styles */ +a { + color: var(--footer-link-color); + font-size: var(--footer-text-size); + transition: text-decoration 0.2s; +} + +a:hover { + text-decoration: underline; + color: var(--footer-link-color); +} -/* Remove duplicate styles from .footer-links a, .copyright a, and their hover states */src/components/footer/footer.component.ts (5)
6-17: Documentation: Add usage examplesThe JSDoc documentation is comprehensive for styling and customization. Consider enhancing it with:
- Basic usage example
- Example of CSS property customization
/** * @element lfx-footer * @summary A footer component for LFX applications * @description This component provides a consistent footer across LFX applications + * + * @example + * ```html + * <lfx-footer></lfx-footer> + * ``` + * + * @example + * ```css + * lfx-footer { + * --lfx-footer-bg: #f5f5f5; + * --lfx-footer-text: #333333; + * } + * ``` * * @csspart footer-container - The main container of the footer
Line range hint
17-22: Consider adding customization optionsThe component could be more reusable by allowing customization of links and copyright text through properties.
export class LFXFooter extends HTMLElement { + static get observedAttributes() { + return ['links', 'copyright-text']; + } + + private _links: Array<{href: string, text: string}> = [ + { href: 'https://www.linuxfoundation.org/legal/privacy-policy?hsLang=en', text: 'Privacy Policy' }, + { href: 'https://www.linuxfoundation.org/legal/trademark-usage?hsLang=en', text: 'Trademark Usage' } + ]; + constructor() { super(); this.attachShadow({ mode: 'open' }); }
Line range hint
24-44: Address accessibility and security concerns
- The separator spans lack semantic meaning and might cause issues with screen readers
- Using innerHTML with template literals could pose XSS risks if properties are added later
- Consider using CSS for separators instead of span elements
- <div class="footer-links"> - <a href="https://www.linuxfoundation.org/legal/privacy-policy?hsLang=en">Privacy Policy</a> - <span>|</span> - <a href="https://www.linuxfoundation.org/legal/trademark-usage?hsLang=en">Trademark Usage</a> - </div> + <nav class="footer-links" aria-label="Footer Navigation"> + <ul> + <li><a href="https://www.linuxfoundation.org/legal/privacy-policy?hsLang=en">Privacy Policy</a></li> + <li><a href="https://www.linuxfoundation.org/legal/trademark-usage?hsLang=en">Trademark Usage</a></li> + </ul> + </nav>Add this CSS to footer.style.ts:
.footer-links ul { list-style: none; padding: 0; margin: 0; display: flex; } .footer-links li:not(:last-child)::after { content: '|'; margin: 0 0.5em; }
Line range hint
46-50: Improve lifecycle handling and DOM manipulation
- Add disconnectedCallback for proper cleanup
- Use safer DOM manipulation methods instead of innerHTML
connectedCallback() { if (this.shadowRoot) { - this.shadowRoot.innerHTML = this.getTemplate(); + const template = document.createElement('template'); + template.innerHTML = this.getTemplate(); + this.shadowRoot.appendChild(template.content.cloneNode(true)); } } + + disconnectedCallback() { + // Clean up any event listeners or resources here + if (this.shadowRoot) { + this.shadowRoot.innerHTML = ''; + } + }
Line range hint
52-58: Add error handling and debugging supportThe component registration is solid but could benefit from error handling and development logging.
(() => { if (typeof window !== 'undefined') { - if (!customElements.get('lfx-footer')) { - customElements.define('lfx-footer', LFXFooter); + try { + if (!customElements.get('lfx-footer')) { + customElements.define('lfx-footer', LFXFooter); + if (process.env.NODE_ENV === 'development') { + console.debug('LFXFooter component registered successfully'); + } + } + } catch (error) { + console.error('Failed to register LFXFooter component:', error); } } })();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/footer/footer.component.ts(2 hunks)src/components/footer/footer.style.ts(1 hunks)
🔇 Additional comments (1)
src/components/footer/footer.style.ts (1)
1-73: Verify style integration with web component
Since these styles are used within a web component, let's verify the integration.
✅ Verification successful
Style integration is properly implemented in the web component
The styles are correctly integrated within the web component:
- Styles are imported from
footer.style.tsand applied via shadow DOM - The component uses
attachShadow({ mode: 'open' })for proper style encapsulation - CSS custom properties are properly defined for theming (
--lfx-footer-bg,--lfx-footer-text) - No conflicting footer styles found in other CSS files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the styles are properly imported and used in the component
ast-grep --pattern 'class LFXFooter extends HTMLElement {
$$$
style = $_;
$$$
}'
# Check for any other footer-related styles that might conflict
rg -g '!*.{md,json}' -i 'footer' --type css
Length of output: 281
Script:
#!/bin/bash
# Let's first find the component file
fd -e ts -e js "footer" --type file
# Then check the content of any found files to verify style integration
fd -e ts -e js "footer" --type file -x cat {}
# Look for any CSS files that might contain footer styles
fd -e css -e scss "footer" --type file
Length of output: 4028
Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
This pull request introduces Storybook integration and improves the documentation for the footer component. The most important changes include adding Storybook configuration files, updating the
package.jsonto include Storybook commands and dependencies, and enhancing the documentation for development purposes.Storybook Integration:
.storybook/main.ts: Added Storybook configuration file to define stories, addons, and framework options..storybook/preview.ts: Added Storybook preview configuration with parameters for actions and controls.package.json: Updated to include Storybook commands (storybookandbuild-storybook) and added necessary Storybook dependencies.Documentation Enhancements:
docs/footer.md: Added a new section on development, including commands to build the component and start the Storybook server.Component Improvements:
src/components/footer/footer.component.ts: Added JSDoc comments to describe the footer component and its CSS parts and properties.Summary by CodeRabbit
New Features
LFXFooterweb component with customizable styles and a shadow DOM.lfx-footercomponent, enabling visual testing and documentation.Documentation
Chores