Skip to content
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.

Sign up for GitHub

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

chore: use lit for shared components #11

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

chore: use lit for shared components #11

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17791c3
chore: update shared components to use lit
asithade Dec 9, 2024
41b6e65
update font colors
asithade Dec 9, 2024
1247aa9
Merge branch 'main' into chore/use-lit
asithade Dec 9, 2024
00ba8e2
fix tsconfig.json
asithade Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"author": "The Linux Foundation",
"scripts": {
"build:tokens": "ts-node src/scripts/build.ts",
"build:browser": "browserify src/components/footer/footer.component.ts -p [ tsify ] -o dist/browser/footer.bundle.js",
"build:browser": "ts-node src/scripts/build-components.ts",
"build": "npm run build:tokens && npm run format && tsc && npm run build:browser",
"prepare": "npm run build",
"format": "prettier --write \"src/**/*.{ts,js,json,md}\"",
Expand All @@ -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",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential version mismatch - browserify dependency missing

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 browserify as 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

"@types/node": "^20.0.0",
"@types/prettier": "3.0.0",
"browserify": "^17.0.0",
Expand Down
46 changes: 16 additions & 30 deletions src/components/footer/footer.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright The Linux Foundation and each contributor to LFX.
// SPDX-License-Identifier: MIT

import { html, LitElement } from 'lit';
import { customElement } from 'lit/decorators.js';

import { style } from './footer.style';

/**
Expand All @@ -15,43 +18,26 @@ import { style } from './footer.style';
* @cssproperty --lfx-footer-bg - Background color of the footer
* @cssproperty --lfx-footer-text - Text color of the footer
*/
export class LFXFooter extends HTMLElement {
constructor() {
super();
this.attachShadow({ mode: 'open' });
}

private getTemplate(): string {
return `
<style>${style}</style>
@customElement('lfx-footer')
export class LFXFooter extends LitElement {
static styles = style;

render() {
return html`
<div class="footer-container">
<div class="footer-content">
<div class="copyright-container">
<p class="copyright">Copyright &copy; ${new Date().getFullYear()} The Linux Foundation&reg;. All rights reserved.
The Linux Foundation has registered trademarks and uses trademarks. For more information, including terms of use,
<a href="https://www.linuxfoundation.org/legal/platform-use-agreement/" target="_blank" rel="noopener noreferrer">Platform Usage</a>,
<a href="https://www.linuxfoundation.org/legal/privacy-policy?hsLang=en" target="_blank" rel="noopener noreferrer">Privacy Policy</a>,
and <a href="https://www.linuxfoundation.org/legal/trademark-usage?hsLang=en" target="_blank" rel="noopener noreferrer">Trademark Usage</a>,
please see our <a href="https://www.linuxfoundation.org/legal/policies" target="_blank" rel="noopener noreferrer">Policies</a> page.</p>
<p class="copyright">
Copyright &copy; ${new Date().getFullYear()} The Linux Foundation&reg;. All rights reserved. The Linux Foundation has registered trademarks and
uses trademarks. For more information, including terms of use,
<a href="https://www.linuxfoundation.org/legal/platform-use-agreement/" target="_blank" rel="noopener noreferrer">Platform Usage</a>,
<a href="https://www.linuxfoundation.org/legal/privacy-policy?hsLang=en" target="_blank" rel="noopener noreferrer">Privacy Policy</a>, and
<a href="https://www.linuxfoundation.org/legal/trademark-usage?hsLang=en" target="_blank" rel="noopener noreferrer">Trademark Usage</a>, please
see our <a href="https://www.linuxfoundation.org/legal/policies" target="_blank" rel="noopener noreferrer">Policies</a> page.
</p>
</div>
</div>
</div>
`;
}

connectedCallback() {
if (this.shadowRoot) {
this.shadowRoot.innerHTML = this.getTemplate();
}
}
}

// Use IIFE to register the component immediately
(() => {
if (typeof window !== 'undefined') {
if (!customElements.get('lfx-footer')) {
customElements.define('lfx-footer', LFXFooter);
}
}
})();
115 changes: 61 additions & 54 deletions src/components/footer/footer.style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,58 +1,65 @@
// Copyright The Linux Foundation and each contributor to LFX.
// SPDX-License-Identifier: MIT

export const style = `
@import url('https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;600&display=swap');

:host {
display: block;
background: var(--lfx-footer-bg, transparent);
padding: 3rem 2rem 0 2rem;
color: var(--lfx-footer-text, #5b6367);
font-family: 'Open Sans', sans-serif;
}

:host * {
margin: 0;
padding: 0;
box-sizing: border-box;
font-size: 0.75rem;
color: #808b91;
text-decoration: none;
}

.footer-container {
margin: 0 auto;
display: flex;
justify-content: center;
align-items: center;
}

.footer-content {
text-align: center;
font-size: 0.75rem;
display: flex;
justify-content: center;
align-items: center;
flex-direction: column;
gap: 1rem;
}

.copyright-container {
display: flex;
flex-direction: column;
}

.copyright {
font-size: 0.75rem;
}

.copyright a {
color: #5b6367;
}

.copyright a:hover {
text-decoration: underline;
color: #5b6367;
}
import { css } from 'lit';

export const style = css`
@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;
}
Comment on lines +7 to +12
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. 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');
  1. 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.

Suggested change
@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');


:host {
display: block;
background: var(--lfx-footer-bg, transparent);
padding: 3rem 2rem 0 2rem;
color: var(--lfx-footer-text, #808b91);
font-family: 'Open Sans', sans-serif;
}

:host * {
margin: 0;
padding: 0;
box-sizing: border-box;
font-size: 0.75rem;
color: var(--lfx-footer-text, #808b91);
text-decoration: none;
}

.footer-container {
margin: 0 auto;
display: flex;
justify-content: center;
align-items: center;
}

.footer-content {
text-align: center;
font-size: 0.75rem;
display: flex;
justify-content: center;
align-items: center;
flex-direction: column;
gap: 1rem;
}

.copyright-container {
display: flex;
flex-direction: column;
}

.copyright {
font-size: 0.75rem;
}

.copyright a {
color: var(--lfx-footer-text, #5b6367);
}

.copyright a:hover {
text-decoration: underline;
color: var(--lfx-footer-text, #5b6367);
}
`;
2 changes: 1 addition & 1 deletion src/design/tokens/tokens.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50608,4 +50608,4 @@
"Aura/Dark"
]
}
}
}
18 changes: 18 additions & 0 deletions src/scripts/build-components.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import browserify from 'browserify';
import fs from 'fs';
import path from 'path';

const components = ['footer/footer.component.ts'];

components.forEach((component) => {
const componentName = path.basename(component, '.ts');
const bundlePath = `dist/browser/${componentName}.bundle.js`;

browserify(`src/components/${component}`)
.plugin('tsify')
.bundle()
.pipe(fs.createWriteStream(bundlePath))
.on('finish', () => {
console.log(`Built ${bundlePath}`);
});
});
Comment on lines +11 to +18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and build optimizations.

The current implementation lacks error handling and build optimizations. Consider these improvements:

  1. Error handling for file operations and bundling
  2. Source maps for debugging
  3. Minification for production builds
  4. 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"
  }
}

4 changes: 3 additions & 1 deletion src/scripts/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
"moduleResolution": "node",
"sourceMap": true,
"outDir": "../../dist/scripts",
"resolveJsonModule": true
"resolveJsonModule": true,
"experimentalDecorators": true,
"emitDecoratorMetadata": false
},
"include": ["./**/*"],
"exclude": ["node_modules"]
Expand Down
5 changes: 3 additions & 2 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
"esModuleInterop": true,
"skipLibCheck": true,
"forceConsistentCasingInFileNames": true,
"rootDir": "./src"
},
"rootDir": "./src",
"experimentalDecorators": true,
"emitDecoratorMetadata": false, },
"include": ["src/**/*"],
"exclude": ["node_modules", "dist"]
}
Loading