Conversation
WalkthroughThe pull request updates the version scheme across the project by changing version numbers from "2.1.0" to "2.1.1-showcase-gallery.0" in several configuration and package files. In addition, dependency versions for packages relying on Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SG as ShowcaseGallery
participant SB as Sidebar
participant IF as IFrame
Note over SG: Component Initialization (default showcase activated)
U->>SG: Load page/connect
U->>SG: Click to select a showcase
SG->>SG: Update active showcase state
alt Screen width small
SG->>SB: Toggle sidebar visibility
end
SG->>IF: Load selected showcase content
IF-->>SG: Notify iframe load complete
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 5
🧹 Nitpick comments (7)
showcase/showcase-gallery/index.html (1)
1-14: New Showcase Gallery HTML File.
The new HTML file sets up a proper document structure with standard meta tags and embeds the<showcase-gallery>custom element. Consider adding a trailing newline if your project's style guidelines require it.showcase/showcase-gallery/package.json (1)
3-3: Consider aligning version with other packagesThe version is set to "0.0.0" while other packages are using "2.1.1-showcase-gallery.0". If this is part of the same monorepo and release cycle, consider aligning versions for consistency.
showcase/showcase-gallery/index.js (3)
6-22: Consider externalizing showcase configurationThe showcase URLs are hardcoded in the component. For better maintainability, consider:
- Accepting the showcases list as a property from outside the component
- Or loading the list from a configuration file/endpoint
This would make it easier to add new showcases without modifying the component code.
export class ShowcaseGallery extends LitElement { @property({ type: Array }) - showcases = [ - { - id: "formula-editor", - name: "Formula Editor", - url: "https://getfundwave.github.io/fw-components/formula-editor/" - }, - { - id: "fw-dnd", - name: "Drag and Drop", - url: "https://getfundwave.github.io/fw-components/fw-dnd/" - }, - { - id: "fw-theme-builder", - name: "Theme Builder", - url: "https://getfundwave.github.io/fw-components/fw-theme-builder/" - } - ]; + showcases = []; + + @property({ type: String }) + configSrc = ''; + + async connectedCallback() { + super.connectedCallback(); + + // If configSrc is provided, load showcases from it + if (this.configSrc) { + try { + const response = await fetch(this.configSrc); + this.showcases = await response.json(); + } catch (error) { + console.error('Failed to load showcases:', error); + // Fallback to default showcases + this.showcases = [ + { + id: "formula-editor", + name: "Formula Editor", + url: "https://getfundwave.github.io/fw-components/formula-editor/" + }, + // ... other default showcases + ]; + } + } else { + // Use default showcases if no configSrc provided + this.showcases = [ + { + id: "formula-editor", + name: "Formula Editor", + url: "https://getfundwave.github.io/fw-components/formula-editor/" + }, + // ... other default showcases + ]; + } + + // Set the first showcase as active by default + if (this.showcases.length > 0 && !this.activeShowcase) { + this.activeShowcase = this.showcases[0]; + this.iframeLoading = true; + } + }
46-48: Add window resize listener for responsive behaviorThe sidebar collapse behavior is based on window width at selection time, but doesn't update if the window is resized. Consider adding a resize listener to manage the sidebar state.
connectedCallback() { super.connectedCallback(); // Set the first showcase as active by default if (this.showcases.length > 0 && !this.activeShowcase) { this.activeShowcase = this.showcases[0]; this.iframeLoading = true; } + + // Set initial sidebar state based on screen size + this.sidebarCollapsed = window.innerWidth <= 768; + + // Add resize listener + this.resizeHandler = () => { + // Only automatically collapse on small screens + if (window.innerWidth <= 768) { + this.sidebarCollapsed = true; + } + }; + window.addEventListener('resize', this.resizeHandler); } + + disconnectedCallback() { + super.disconnectedCallback(); + // Clean up event listener + window.removeEventListener('resize', this.resizeHandler); + }
59-226: Consider moving styles to a separate fileThe CSS styles are quite lengthy. For better maintainability, consider:
- Moving them to a separate CSS file and importing them
- Or organizing them into themed sections with comments
This would make the component file more focused on behavior rather than styling.
showcase/showcase-gallery/webpack.js (2)
1-1: Consider using specific eslint-disable directives instead of disabling all rulesRather than disabling all ESLint rules for the entire file, it's better to disable only specific rules where needed. This maintains code quality while addressing specific exceptions.
-/* eslint-disable */ +/* eslint-disable import/no-commonjs, import/no-extraneous-dependencies */
100-106: Make the dev server port configurable via environment variablesHard-coding the port to 8080 might cause conflicts with other services. Consider making it configurable:
devServer: { static: path.join(__dirname, "./dist"), compress: true, - port: 8080, + port: process.env.PORT || 8080, historyApiFallback: true, hot: true, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/fw-theme-builder/package-lock.jsonis excluded by!**/package-lock.jsonshowcase/showcase-gallery/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
lerna.json(1 hunks)packages/formula-editor/package.json(2 hunks)packages/fw-avatar/package.json(2 hunks)packages/fw-dnd/package.json(2 hunks)packages/fw-theme-builder/package.json(2 hunks)packages/styles/package.json(1 hunks)packages/trackers/package.json(1 hunks)showcase/showcase-gallery/index.html(1 hunks)showcase/showcase-gallery/index.js(1 hunks)showcase/showcase-gallery/package.json(1 hunks)showcase/showcase-gallery/webpack.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
showcase/showcase-gallery/webpack.js
[error] 84-86: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🔇 Additional comments (16)
packages/trackers/package.json (1)
3-3: Version Bump Confirmation.
The package version is updated to"2.1.1-showcase-gallery.0"as required by the showcase gallery release. Ensure that this change is consistently reflected across all interdependent packages.lerna.json (1)
6-6: Lerna Version Synchronization.
The version field is updated to"2.1.1-showcase-gallery.0", which is necessary for consistent release management within the monorepo.packages/styles/package.json (1)
3-3: Consistent Styles Package Version.
The"version"field has been correctly updated to"2.1.1-showcase-gallery.0"to align with the new release scheme for the showcase gallery.packages/formula-editor/package.json (2)
3-3: Formula Editor Version Update.
The package version is updated to"2.1.1-showcase-gallery.0", ensuring consistency with the overall release update.
22-22: Dependency Version Alignment.
The dependency on"@fw-components/styles"has been updated to"^2.1.1-showcase-gallery.0", aligning it with the new versioning across the monorepo.packages/fw-avatar/package.json (2)
3-3: Version update follows semver principlesThe version change to include the prerelease tag "-showcase-gallery.0" is appropriate for this feature branch.
18-18: Dependency version updated correctlyGood practice to keep the styles dependency version aligned with the component version.
packages/fw-dnd/package.json (2)
3-3: Version updated consistentlyThe version update matches the pattern used in other components, maintaining consistency across the project.
15-15: Styles dependency updated correctlyThe dependency update ensures compatibility with other components in this feature branch.
packages/fw-theme-builder/package.json (2)
3-3: Version updated consistentlyVersion update follows the same pattern as other components in this PR.
17-17: Styles dependency updated appropriatelyThe dependency version is correctly updated to maintain compatibility with the other components.
showcase/showcase-gallery/package.json (1)
6-9: Webpack scripts look goodThe webpack configuration scripts for development and building are properly set up.
showcase/showcase-gallery/index.js (2)
33-40: Component structure looks goodThe component lifecycle management and initialization logic are well implemented.
228-278: Well-structured render methodThe render method is clear and organized, with good conditional rendering for different states.
showcase/showcase-gallery/webpack.js (2)
25-39: Consider the implications of usingloose: truefor Babel pluginsThe configuration uses
{ loose: true }for multiple Babel plugins. While this can improve performance and reduce bundle size, loose mode generates code that's not strictly compliant with ECMAScript specifications, which could cause compatibility issues.Only use loose mode if you've confirmed it doesn't affect your target environments negatively, or consider documenting why this decision was made.
10-129: Overall webpack configuration looks appropriate for the showcase galleryThe webpack configuration appears well-structured for a showcase gallery application, with proper handling of various asset types and appropriate development tooling. The entry point, output configuration, and plugins chosen are all suitable for this purpose.
🧰 Tools
🪛 Biome (1.9.4)
[error] 84-86: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
| "webpack-dev": "webpack --config webpack.js", | ||
| "start": "webpack-dev-server --config webpack.js --host 0.0.0.0 --hot", | ||
| "build": "webpack --config webpack.js --stats verbose", | ||
| "test": "echo \"Error: no test specified\" && exit 1" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the test script
The current test script will exit with error code 1, which could cause CI pipelines to fail if they run tests on all packages.
- "test": "echo \"Error: no test specified\" && exit 1"
+ "test": "echo \"No tests specified for showcase-gallery\" && exit 0"📝 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.
| "test": "echo \"Error: no test specified\" && exit 1" | |
| "test": "echo \"No tests specified for showcase-gallery\" && exit 0" |
| <div | ||
| class="showcase-item ${this.activeShowcase && this.activeShowcase.id === showcase.id ? 'active' : ''}" | ||
| @click=${() => this.selectShowcase(showcase)} | ||
| > | ||
| <div class="showcase-name">${showcase.name}</div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard accessibility to showcase items
The showcase items should be keyboard accessible. Consider adding tabindex, keyboard event handling, and appropriate ARIA attributes.
<div
class="showcase-item ${this.activeShowcase && this.activeShowcase.id === showcase.id ? 'active' : ''}"
@click=${() => this.selectShowcase(showcase)}
+ tabindex="0"
+ role="button"
+ aria-pressed="${this.activeShowcase && this.activeShowcase.id === showcase.id ? 'true' : 'false'}"
+ @keydown=${(e) => e.key === 'Enter' && this.selectShowcase(showcase)}
>
<div class="showcase-name">${showcase.name}</div>
</div>📝 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.
| <div | |
| class="showcase-item ${this.activeShowcase && this.activeShowcase.id === showcase.id ? 'active' : ''}" | |
| @click=${() => this.selectShowcase(showcase)} | |
| > | |
| <div class="showcase-name">${showcase.name}</div> | |
| </div> | |
| <div | |
| class="showcase-item ${this.activeShowcase && this.activeShowcase.id === showcase.id ? 'active' : ''}" | |
| @click=${() => this.selectShowcase(showcase)} | |
| tabindex="0" | |
| role="button" | |
| aria-pressed="${this.activeShowcase && this.activeShowcase.id === showcase.id ? 'true' : 'false'}" | |
| @keydown=${(e) => e.key === 'Enter' && this.selectShowcase(showcase)} | |
| > | |
| <div class="showcase-name">${showcase.name}</div> | |
| </div> |
| <button class="toggle-sidebar ${this.sidebarCollapsed ? 'collapsed' : ''}" @click=${this.toggleSidebar}> | ||
| ${this.sidebarCollapsed | ||
| ? html`<svg viewBox="0 0 24 24"><path d="M8.59,16.59L13.17,12L8.59,7.41L10,6l6,6l-6,6L8.59,16.59z"/></svg>` | ||
| : html`<svg viewBox="0 0 24 24"><path d="M15.41,16.59L10.83,12l4.58-4.59L14,6l-6,6l6,6L15.41,16.59z"/></svg>` | ||
| } | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to interactive elements
The toggle button lacks appropriate accessibility attributes. Add aria labels and roles where needed.
- <button class="toggle-sidebar ${this.sidebarCollapsed ? 'collapsed' : ''}" @click=${this.toggleSidebar}>
+ <button
+ class="toggle-sidebar ${this.sidebarCollapsed ? 'collapsed' : ''}"
+ @click=${this.toggleSidebar}
+ aria-label="${this.sidebarCollapsed ? 'Show sidebar' : 'Hide sidebar'}"
+ aria-expanded="${!this.sidebarCollapsed}">
${this.sidebarCollapsed
? html`<svg viewBox="0 0 24 24"><path d="M8.59,16.59L13.17,12L8.59,7.41L10,6l6,6l-6,6L8.59,16.59z"/></svg>`
: html`<svg viewBox="0 0 24 24"><path d="M15.41,16.59L10.83,12l4.58-4.59L14,6l-6,6l6,6L15.41,16.59z"/></svg>`
}
</button>📝 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.
| <button class="toggle-sidebar ${this.sidebarCollapsed ? 'collapsed' : ''}" @click=${this.toggleSidebar}> | |
| ${this.sidebarCollapsed | |
| ? html`<svg viewBox="0 0 24 24"><path d="M8.59,16.59L13.17,12L8.59,7.41L10,6l6,6l-6,6L8.59,16.59z"/></svg>` | |
| : html`<svg viewBox="0 0 24 24"><path d="M15.41,16.59L10.83,12l4.58-4.59L14,6l-6,6l6,6L15.41,16.59z"/></svg>` | |
| } | |
| </button> | |
| <button | |
| class="toggle-sidebar ${this.sidebarCollapsed ? 'collapsed' : ''}" | |
| @click=${this.toggleSidebar} | |
| aria-label="${this.sidebarCollapsed ? 'Show sidebar' : 'Hide sidebar'}" | |
| aria-expanded="${!this.sidebarCollapsed}"> | |
| ${this.sidebarCollapsed | |
| ? html`<svg viewBox="0 0 24 24"><path d="M8.59,16.59L13.17,12L8.59,7.41L10,6l6,6l-6,6L8.59,16.59z"/></svg>` | |
| : html`<svg viewBox="0 0 24 24"><path d="M15.41,16.59L10.83,12l4.58-4.59L14,6l-6,6l6,6L15.41,16.59z"/></svg>` | |
| } | |
| </button> |
| handleIframeLoad() { | ||
| this.iframeLoading = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for iframe loading
The component doesn't handle cases where the iframe fails to load. Consider adding error handling.
+ @state()
+ iframeError = false;
handleIframeLoad() {
this.iframeLoading = false;
+ this.iframeError = false;
+ }
+
+ handleIframeError() {
+ this.iframeLoading = false;
+ this.iframeError = true;
}
// Then in the render method:
// <iframe
// src="${this.activeShowcase.url}"
// @load=${this.handleIframeLoad}
+ // @error=${this.handleIframeError}
// title="${this.activeShowcase.name}"
// ></iframe>
// And add a display for errors:
// ${this.iframeError
// ? html`
// <div class="error-indicator">
// <div class="error-message">
// Failed to load content. Please try again later.
// </div>
// </div>
// `
// : ``}📝 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.
| handleIframeLoad() { | |
| this.iframeLoading = false; | |
| } | |
| @state() | |
| iframeError = false; | |
| handleIframeLoad() { | |
| this.iframeLoading = false; | |
| this.iframeError = false; | |
| } | |
| handleIframeError() { | |
| this.iframeLoading = false; | |
| this.iframeError = true; | |
| } | |
| // Then in the render method: | |
| // <iframe | |
| // src="${this.activeShowcase.url}" | |
| // @load=${this.handleIframeLoad} | |
| // @error=${this.handleIframeError} | |
| // title="${this.activeShowcase.name}" | |
| // ></iframe> | |
| // And add a display for errors: | |
| // ${this.iframeError | |
| // ? html` | |
| // <div class="error-indicator"> | |
| // <div class="error-message"> | |
| // Failed to load content. Please try again later. | |
| // </div> | |
| // </div> | |
| // ` | |
| // : ``} |
| test: new RegExp( | ||
| "./node_modules/lit-element-router/utility/router-utility.js" | ||
| ), | ||
| loader: "string-replace-loader", | ||
| options: { | ||
| search: /\[\\\\w\\u00C0-\\u00D6\\u00D8-\\u00f6\\u00f8-\\u00ff-\]/, | ||
| replace: "[^/]", | ||
| strict: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a regular expression literal instead of the RegExp constructor
Using the RegExp constructor requires double escaping, which makes the pattern harder to read and maintain. A literal regex would be clearer and less error-prone.
- test: new RegExp(
- "./node_modules/lit-element-router/utility/router-utility.js"
- ),
+ test: /\.\/node_modules\/lit-element-router\/utility\/router-utility\.js/,
loader: "string-replace-loader",
options: {
- search: /\[\\\\w\\u00C0-\\u00D6\\u00D8-\\u00f6\\u00f8-\\u00ff-\]/,
+ search: /\[\\w\u00C0-\u00D6\u00D8-\u00f6\u00f8-\u00ff-\]/,
replace: "[^/]",
strict: true,
},📝 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.
| test: new RegExp( | |
| "./node_modules/lit-element-router/utility/router-utility.js" | |
| ), | |
| loader: "string-replace-loader", | |
| options: { | |
| search: /\[\\\\w\\u00C0-\\u00D6\\u00D8-\\u00f6\\u00f8-\\u00ff-\]/, | |
| replace: "[^/]", | |
| strict: true, | |
| }, | |
| }, | |
| test: /\.\/node_modules\/lit-element-router\/utility\/router-utility\.js/, | |
| loader: "string-replace-loader", | |
| options: { | |
| search: /\[\\w\u00C0-\u00D6\u00D8-\u00f6\u00f8-\u00ff-\]/, | |
| replace: "[^/]", | |
| strict: true, | |
| }, | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 84-86: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
1 participant
Description
Add showcase gallery for fw-components
Summary by CodeRabbit
New Features
Chores