chore: use lit for shared components #11
Conversation
Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
WalkthroughThe pull request introduces several significant changes across multiple files. The Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (3)
src/scripts/build-components.ts (2)
5-5: Consider dynamic component discovery.Instead of hardcoding component paths, consider using the filesystem to discover components. This would make the build process more maintainable as new components are added.
-const components = ['footer/footer.component.ts']; +const componentsDir = path.join(__dirname, '../components'); +const components = fs.readdirSync(componentsDir, { recursive: true }) + .filter((file: string) => file.endsWith('.component.ts')) + .map(file => path.relative(componentsDir, file));
1-4: Consider adding configuration options.The build script could benefit from configuration options to control:
- Development vs production builds
- Source map generation
- Output directory
- Component inclusion/exclusion patterns
Consider extracting configuration to a separate file:
// build-config.ts export interface BuildConfig { production: boolean; sourceMap: boolean; outDir: string; componentPatterns: string[]; } export const defaultConfig: BuildConfig = { production: process.env.NODE_ENV === 'production', sourceMap: true, outDir: 'dist/browser', componentPatterns: ['**/*.component.ts'] };src/components/footer/footer.component.ts (1)
21-23: Consider declaring CSS custom propertiesFor better TypeScript support and documentation, consider declaring the CSS custom properties as static properties.
@customElement('lfx-footer') export class LFXFooter extends LitElement { static styles = style; + + static properties = { + '--lfx-footer-bg': { type: String }, + '--lfx-footer-text': { type: String } + };
📜 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 (7)
package.json(2 hunks)src/components/footer/footer.component.ts(2 hunks)src/components/footer/footer.style.ts(1 hunks)src/design/tokens/tokens.json(1 hunks)src/scripts/build-components.ts(1 hunks)src/scripts/tsconfig.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/design/tokens/tokens.json
🔇 Additional comments (6)
tsconfig.json (1)
11-13: LGTM! Configuration is appropriate for Lit.
The TypeScript configuration changes are correct for using Lit decorators. Setting experimentalDecorators: true enables the @customElement decorator support while keeping emitDecoratorMetadata: false is appropriate as Lit doesn't require runtime type information.
Let's verify that all component files using decorators are under the configured rootDir:
✅ Verification successful
Configuration is correctly aligned with decorator usage
All TypeScript files, including the Lit component using @customElement decorator, are properly located under the configured src/ directory. The decorator is used only in footer.component.ts with appropriate Lit imports, which aligns perfectly with the TypeScript configuration settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all TypeScript files using the @customElement decorator
rg "@customElement" --type ts
# Verify they are all under src/
fd ".ts$" | grep -v "^src/"
Length of output: 130
Script:
#!/bin/bash
# Let's verify the decorator usage and imports more thoroughly
rg "customElement" --type ts -A 2
# Also check for any Lit-related imports
rg "from ['|\"]lit" --type ts
# Double check all TypeScript files location
fd -e ts
Length of output: 1122
src/scripts/tsconfig.json (1)
12-14: LGTM! Configuration maintains consistency.
The TypeScript configuration appropriately mirrors the decorator settings from the root config while maintaining script-specific settings like resolveJsonModule.
package.json (1)
11-11: LGTM: Improved build script organization
Moving the build logic to a dedicated TypeScript file improves maintainability and allows for better error handling and configuration.
src/components/footer/footer.style.ts (1)
14-64: LGTM: Well-structured CSS with proper theming support
The CSS implementation follows best practices:
- Proper use of CSS variables with fallbacks for theming
- Well-scoped selectors using :host
- Consistent styling patterns
src/components/footer/footer.component.ts (2)
Line range hint 9-20: LGTM: Well-documented component API
The JSDoc documentation clearly describes the component's purpose, parts, and CSS properties.
25-42: LGTM: Clean and secure implementation
The render method follows best practices:
- Proper use of lit's html template literal
- External links include security attributes (target, rel)
- Dynamic year calculation
| browserify(`src/components/${component}`) | ||
| .plugin('tsify') | ||
| .bundle() | ||
| .pipe(fs.createWriteStream(bundlePath)) | ||
| .on('finish', () => { | ||
| console.log(`Built ${bundlePath}`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and build optimizations.
The current implementation lacks error handling and build optimizations. Consider these improvements:
- Error handling for file operations and bundling
- Source maps for debugging
- Minification for production builds
- Cleanup of existing bundles
Here's a suggested implementation:
+ // Ensure output directory exists
+ fs.mkdirSync(path.dirname(bundlePath), { recursive: true });
+
browserify(`src/components/${component}`)
.plugin('tsify')
+ .plugin('errorify') // Add error reporting
+ .transform('uglifyify', { sourceMap: true }) // Add minification
.bundle()
+ .on('error', (err) => {
+ console.error(`Error building ${componentName}:`, err);
+ process.exit(1);
+ })
.pipe(fs.createWriteStream(bundlePath))
.on('finish', () => {
console.log(`Built ${bundlePath}`);
- });
+ })
+ .on('error', (err) => {
+ console.error(`Error writing ${bundlePath}:`, err);
+ process.exit(1);
+ });Don't forget to add these dependencies to package.json:
{
"devDependencies": {
"errorify": "^1.0.0",
"uglifyify": "^5.0.2"
}
}| @@ -25,6 +25,7 @@ | |||
| "@storybook/blocks": "^8.4.5", | |||
| "@storybook/web-components": "^8.4.5", | |||
| "@storybook/web-components-vite": "^8.4.5", | |||
| "@types/browserify": "^12.0.40", | |||
There was a problem hiding this comment.
💡 Codebase verification
The @types/browserify@^12.0.40 types package is added as a dev dependency, but there's no corresponding browserify package in the dependencies. This could lead to runtime issues as the types may not match the actual implementation.
- Add
browserifyas a direct dependency in package.json - Ensure the browserify version is compatible with the types package (v12.x.x)
🔗 Analysis chain
Verify browserify version compatibility
The added @types/browserify@^12.0.40 types package should be compatible with the installed browserify@^17.0.0. Let's verify this compatibility.
Also applies to: 31-31
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check browserify types compatibility with installed version
# Get the installed browserify version and types version requirements
echo "Checking browserify package.json..."
cat node_modules/browserify/package.json | jq '.version'
echo "Checking @types/browserify version compatibility..."
cat node_modules/@types/browserify/package.json | jq '.version, ."typescript-version"'
Length of output: 484
Script:
#!/bin/bash
# Let's check the actual versions in package.json first
echo "Checking package.json dependencies..."
cat package.json | jq '.dependencies.browserify, .devDependencies["@types/browserify"]'
# Let's also check if there are any browserify-related files
echo -e "\nChecking for browserify-related files..."
fd browserify
Length of output: 303
| @font-face { | ||
| font-family: 'Open Sans', sans-serif; | ||
| src: url(https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap) format('truetype'); | ||
| font-weight: normal; | ||
| font-style: normal; | ||
| } |
There was a problem hiding this comment.
Fix @font-face declaration for Google Fonts
The current @font-face declaration won't work as intended. Google Fonts should be loaded via a link tag in the HTML or using the CSS @import rule. The current URL points to a CSS file, not a font file.
Consider one of these approaches:
- Using CSS @import (preferred for web components):
- @font-face {
- font-family: 'Open Sans', sans-serif;
- src: url(https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap) format('truetype');
- font-weight: normal;
- font-style: normal;
- }
+ @import url('https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap');- Or remove the @font-face and document that the font needs to be loaded by the consuming application.
📝 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.
| @font-face { | |
| font-family: 'Open Sans', sans-serif; | |
| src: url(https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap) format('truetype'); | |
| font-weight: normal; | |
| font-style: normal; | |
| } | |
| @import url('https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap'); |
Signed-off-by: Asitha de Silva <[email protected]>
This pull request includes significant changes to the build process and the
footercomponent. The most important changes include switching toLitElementfor the footer component, updating the build scripts, and modifying the styles to use thecsstagged template fromlit.Build Process Updates:
package.json: Updated thebuild:browserscript to use a new TypeScript-based build script (src/scripts/build-components.ts). Added@types/browserifyto the dependencies. [1] [2]src/scripts/build-components.ts: Added a new script to bundle components usingbrowserifyandtsify.Footer Component Refactor:
src/components/footer/footer.component.ts: Refactored the footer component to useLitElementand the@customElementdecorator fromlit. [1] [2]src/components/footer/footer.style.ts: Updated styles to use thecsstagged template fromlitand modified the font import to use@font-face. [1] [2] [3]TypeScript Configuration:
src/scripts/tsconfig.json: EnabledexperimentalDecoratorsand disabledemitDecoratorMetadatato support the use of decorators in the project.Summary by CodeRabbit
New Features
LFXFootercomponent with improved rendering capabilities using LitElement.Bug Fixes
Documentation
Chores