Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: timea-solid <4144203+timea-solid@users.noreply.github.com>
Co-authored-by: timea-solid <4144203+timea-solid@users.noreply.github.com>
[WIP] Address feedback on local dev setup pull request
[WIP] Update local dev setup based on review feedback
There was a problem hiding this comment.
Pull request overview
This PR refactors the contacts-pane codebase to separate inline styles into external CSS files, modernize the build system with webpack, and remove unused code related to public data lookup functionality. The changes improve maintainability by making styles more declarative and easier to manage while reducing bundle size by removing the heavy mime-types dependency and unused autocomplete features.
Changes:
- Extracted all inline styles from JavaScript files into separate CSS files in
src/styles/directory using CSS class-based styling with BEM naming conventions - Added webpack-based build configuration for both development and production environments, replacing the previous Babel-only build
- Removed unused public data integration code (
publicData.ts,autocompletePicker.ts,autocompleteBar.ts, etc.) and replaced with solid-ui's built-in widgets - Replaced mime-types package (~170 KiB) with lightweight custom MIME type helpers
- Added development server setup with HTML template and global CSS variables
Reviewed changes
Copilot reviewed 32 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.mjs | New production build configuration with UMD export and CSS/TTL handling |
| webpack.dev.config.mjs | New development server configuration with HtmlWebpackPlugin and hot reloading |
| src/styles/*.css | Five new CSS files extracting inline styles from corresponding JavaScript modules |
| src/contactsPane.js | Converted inline style attributes to classList.add/toggle calls |
| src/webidControl.js | Replaced local autocomplete with solid-ui widgets, converted inline styles to classes |
| src/toolsPane.js | Converted inline styles to CSS classes, removed global comment |
| src/individual.js | Converted inline styles to CSS classes |
| src/mugshotGallery.js | Replaced mime-types dependency with custom lightweight implementation |
| src/mintNewAddressBook.js | Converted inline style to CSS class |
| src/ontology/forms.ttl | Minor spelling and formatting corrections |
| package.json | Updated build scripts, removed mime-types dependency, added webpack tooling |
| jest.config.mjs | Added CSS mock mapping for tests |
| declarations.d.ts | Changed .sparql module declaration to .css |
| babel.config.mjs | Removed .sparql from inline-import extensions |
| dev/* | New development server files (index.ts, index.html, context.ts, dev-global.css) |
| README.md | Expanded with development server and build instructions |
| test/mocks/styleMock.js | New CSS mock for Jest tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "test": "jest --no-coverage", | ||
| "test-coverage": "jest --coverage", | ||
| "prepublishOnly": "npm run build && npm run lint && npm run test", | ||
| "preversion": "npm run lint && npm run typecheck && npm run test", |
There was a problem hiding this comment.
The preversion script references a "typecheck" script that no longer exists (it was removed in the build script changes). This will cause the preversion script to fail. Either remove the typecheck reference or add back the typecheck script.
| const mime = { | ||
| extension: (contentType) => mimeMap[contentType] || false, | ||
| lookup: (filename) => { | ||
| const ext = filename.split('.').pop().toLowerCase() |
There was a problem hiding this comment.
The custom MIME implementation doesn't handle filenames without extensions correctly. When a filename has no extension (e.g., "README"), the split('.').pop() will return the entire filename instead of an extension, potentially causing incorrect MIME type lookups. Consider adding a check to ensure there's actually a period in the filename before attempting extension lookup.
| const ext = filename.split('.').pop().toLowerCase() | |
| if (typeof filename !== 'string') return false | |
| const lastDot = filename.lastIndexOf('.') | |
| // No dot, dot is first character, or dot is last character -> no valid extension | |
| if (lastDot <= 0 || lastDot === filename.length - 1) return false | |
| const ext = filename.slice(lastDot + 1).toLowerCase() |
| .contactPane .toolsTable { | ||
| font-size: 120%; | ||
| margin: var(--spacing-md); | ||
| border: 0.1em var(--color-border-pale); |
There was a problem hiding this comment.
Incomplete CSS border declaration. The border property is missing the style keyword (e.g., 'solid'). It should be something like "border: 0.1em solid var(--color-border-pale);" instead of just "border: 0.1em var(--color-border-pale);"
| border: 0.1em var(--color-border-pale); | |
| border: 0.1em solid var(--color-border-pale); |
| plugins: [ | ||
| ...(common.plugins || []), | ||
| new CopyPlugin({ | ||
| patterns: [ | ||
| { | ||
| from: path.resolve('src/styles'), | ||
| to: path.resolve('dist/styles'), | ||
| }, | ||
| ], | ||
| }), |
There was a problem hiding this comment.
The webpack configuration uses 'style-loader' which injects CSS into the JavaScript bundle. This means CSS will be inlined in the dist/contacts-pane.js file. However, the CopyPlugin also copies CSS files to dist/styles/. This creates confusion about which CSS files consumers should use. Consider either: (1) only bundling CSS into JS (remove CopyPlugin for styles), or (2) extracting CSS to separate files using MiniCssExtractPlugin instead of style-loader, and documenting that consumers must load both JS and CSS files.
| { | ||
| "name": "contacts-pane", | ||
| "version": "3.0.1", | ||
| "version": "3.1.0-newStyle", |
There was a problem hiding this comment.
The version string "3.1.0-newStyle" uses a non-standard prerelease identifier. Semantic versioning typically uses identifiers like "alpha", "beta", "rc", etc. Consider changing to a more conventional format like "3.1.0-alpha" or "3.1.0-beta", or remove the prerelease identifier entirely if this is intended to be a stable release.
| "version": "3.1.0-newStyle", | |
| "version": "3.1.0", |
| # | ||
| # Indivduals and orgs are in one file as they both | ||
| # share some forms (address etc) and also interact (roles) | ||
| # Indivduals and orgs are in one file as they both |
There was a problem hiding this comment.
Typo in comment: "Indivduals" should be "Individuals"
| # Indivduals and orgs are in one file as they both | |
| # Individuals and orgs are in one file as they both |
No description provided.