Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This reverts commit 29ffe34.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR appears aimed at making the source pane styling responsive by moving inline styling into CSS, and it additionally introduces a webpack/babel build + dev-server setup to bundle assets.
Changes:
- Add responsive CSS for the source pane and switch from inline styles to CSS classes.
- Introduce webpack configs (prod + dev) and Babel config to build/bundle the pane (including CSS handling).
- Adjust tests to accommodate the new module export shape.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/sourcePane.js |
Converts to ESM, imports CSS, replaces inline style/visibility logic with CSS class toggles. |
src/styles/sourcePane.css |
Adds responsive layout rules and new state/visibility classes for the pane UI. |
webpack.module.rules.mjs |
Adds loader rules for JS/TS, CSS, CSS modules, and .ttl as asset/source. |
webpack.config.mjs |
Adds production bundling to lib/ (normal + min UMD builds) and copies styles. |
webpack.dev.config.mjs |
Adds a dev-server config to run the pane locally with HTML template and polyfills. |
babel.config.mjs |
Adds preset-env + inline-import plugin configuration. |
package.json |
Adds sideEffects config for CSS, adds build/clean/start scripts, and adds webpack/babel/CSS tooling deps. |
test/sourcePane.test.js |
Attempts to handle default export when importing the pane module in tests. |
dev/index.js |
Adds a local dev entry to render the pane in the browser. |
dev/index.html |
Adds a simple dev HTML page/template for webpack dev-server. |
dev/dev-global.css |
Adds a large set of dev-only global styles/variables. |
dev/context.js |
Adds a reusable dev context stub for solid-logic/store. |
README.md |
Adds a “Generative AI usage” section. |
.gitignore |
Ignores lib/ build output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "main": "src/sourcePane.js", | ||
| "files": [ | ||
| "/src", | ||
| "README.md", | ||
| "LICENSE" | ||
| ], |
There was a problem hiding this comment.
The build now outputs bundles into lib/ (webpack config), but package.json still publishes only /src via the files allowlist and main still points at src/sourcePane.js. This means the published package will not include the built artifacts and consumers will still load the source entrypoint (which now imports CSS and uses ESM syntax). Either publish lib/ and point main/exports at it, or revert to shipping the source entry without requiring a build step.
| const table = div.appendChild(dom.createElement('table')) | ||
| table.setAttribute('class', 'sourcePaneTable') | ||
| const main = table.appendChild(dom.createElement('tr')) | ||
| main.setAttribute('class', 'sourcePaneMainCell') | ||
| const statusRow = table.appendChild(dom.createElement('tr')) | ||
| statusRow.setAttribute('class', 'sourcePaneStatusRow') | ||
| const controls = table.appendChild(dom.createElement('tr')) | ||
| controls.setAttribute('style', 'text-align: right;') | ||
| controls.setAttribute('class', 'sourcePaneControls sourcePaneControlsCell') | ||
|
|
||
| const textArea = main.appendChild(dom.createElement('textarea')) | ||
| textArea.setAttribute('style', editStyle) | ||
| textArea.setAttribute('class', 'sourcePaneTextArea') |
There was a problem hiding this comment.
The DOM structure is invalid HTML table markup: <tr> elements are being used as containers for <textarea> and buttons without any <td>/<th> cells. This can break layout/accessibility, and it’s especially brittle now that flexbox styles are applied to .sourcePaneControls on a <tr>. Consider replacing the table-based layout with <div>s, or insert proper <td> elements and apply classes to the cells instead of the rows.
| } | ||
|
|
||
| .sourcePaneControlHidden { | ||
| visibility: collapse; |
There was a problem hiding this comment.
.sourcePaneControlHidden uses visibility: collapse, which is only well-defined for table rows/columns; for buttons/other elements behavior is inconsistent and it also preserves layout space (problematic with the new flex layout and gap). Prefer a more reliable hiding strategy such as display: none (or a hidden attribute) if you need the control removed from layout.
| visibility: collapse; | |
| display: none; |
| "version": "3.0.2", | ||
| "description": "Solid-compatible Panes: Source editor", | ||
| "sideEffects": [ | ||
| "*.css" |
There was a problem hiding this comment.
sideEffects is set to ".css", which only matches CSS files at the package root. The imported stylesheet lives under src/styles/, so Webpack/tree-shaking may incorrectly drop it in production builds. Use a recursive glob (e.g. "**/.css" or "src/**/*.css") or explicitly list the actual CSS path(s).
| "*.css" | |
| "**/*.css" |
| "clean": "rm -rf lib", | ||
| "build": "npm run clean && npm run build-dist", | ||
| "build-dist": "webpack --progress", |
There was a problem hiding this comment.
This PR’s title is "responsive css", but it also introduces a new build pipeline (webpack/babel), changes the package’s entrypoints, and changes module format. If the intent is only styling, consider splitting the tooling/packaging changes into a separate PR; otherwise, please update the PR description/title to reflect the broader scope.
| import * as $rdf from 'rdflib' | ||
| import * as UI from 'solid-ui' | ||
| import * as mime from 'mime-types' | ||
| import './styles/sourcePane.css' | ||
|
|
||
| module.exports = { | ||
| const pane = { |
There was a problem hiding this comment.
src/sourcePane.js has been converted to ESM (import/export default) and now imports a CSS file. Since package.json does not set type: "module" and main points at this file, Node/Jest/CommonJS consumers will fail to load it (syntax error / unknown CSS import). Decide on a supported distribution format and align the package entrypoints accordingly (e.g., keep this file CommonJS, or switch the package to ESM and add a CJS wrapper/build output for require(), and ensure CSS is either bundled or documented as a separate import).
| const paneModule = require('../src/sourcePane') | ||
| const pane = paneModule.default || paneModule |
There was a problem hiding this comment.
require('../src/sourcePane') will not work if src/sourcePane.js is ESM (and it now imports a CSS file). Jest currently has no ESM/CSS transform or moduleNameMapper for CSS, so the test suite will fail to load the module. Either keep src/sourcePane.js CommonJS for the published entry and tests, or update the package/Jest configuration to run ESM and stub CSS imports.
| document.getElementById('app').replaceWith(app) | ||
| } catch (err) { | ||
| console.error('Error rendering pane:', err) | ||
| document.getElementById('app').innerHTML = `<p style="color:red;">Error: ${err.message}</p>` |
There was a problem hiding this comment.
err.message is interpolated into innerHTML in the dev page. If an error message contains user-controlled content (e.g., derived from a remote response), this can lead to HTML injection even in local dev. Prefer setting textContent (or constructing DOM nodes) instead of using innerHTML for error rendering.
| document.getElementById('app').innerHTML = `<p style="color:red;">Error: ${err.message}</p>` | |
| const appElement = document.getElementById('app') | |
| const errorParagraph = document.createElement('p') | |
| errorParagraph.style.color = 'red' | |
| errorParagraph.textContent = `Error: ${err.message}` | |
| appElement.textContent = '' | |
| appElement.appendChild(errorParagraph) |
responsice css