Milestone3m bis - #248
Conversation
Milestone3m responsive
There was a problem hiding this comment.
Pull request overview
This PR updates the contacts-pane UI/dev setup to support the newer responsive/mobile layout work, adjusts which subjects the pane advertises itself for, and expands the dev build configuration and styling utilities.
Changes:
- Update responsive behavior and mobile-focused styling across multiple contacts-pane CSS modules and the WebID control.
- Adjust pane labeling behavior (and tests) to reflect the new design intent.
- Improve dev experience: extra dev HTML output, image asset handling in dev webpack, new watch scripts, and expanded dev CSS/utilities.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.dev.config.mjs | Adds a second HTML output and an asset rule for images in dev builds. |
| test/unit/data-reformat-test.test.ts | Skips several label tests impacted by updated label logic. |
| src/webidControl.js | Adds an additional modifier class to the WebID control root element. |
| src/toolsPane.js | Comments out a lint-offending constant-condition block. |
| src/styles/webidControl.css | Adds extensive responsive rules for embedded/narrow profile rendering. |
| src/styles/utilities.css | Removes previously-defined utility helpers (notably .hidden). |
| src/styles/mugshotGallery.css | Switches sizing from em to rem. |
| src/styles/localUtils.css | Adjusts button font-weight to use a variable fallback. |
| src/styles/groupMembership.css | Converts several fixed px/em sizes to rem (with comments). |
| src/styles/contactsRDFFormsEnforced.css | Converts some em/px sizing to rem and uses border/icon vars. |
| src/styles/contactsPane.css | Multiple responsive/layout changes (min-width/min-height, overflow, sticky button section, rem conversions). |
| src/localUtils.js | Refactors responsive stacking logic and adds debounced resize handling. |
| src/contactsPane.js | Changes label() behavior and adds global: false. |
| package.json | Version bump; adds watch scripts; adds file-loader. |
| package-lock.json | Lockfile updates for version bump and added dependency. |
| dev/index.ts | Adds dev utilities CSS import and changes the default webIdToShow. |
| dev/dev-utilities.css | Adds a large utility CSS set for dev overrides (accessibility/layout/components). |
| dev/dev-global.css | Large update to dev global variables/resets to match new design tokens. |
| dev/context.ts | Expands pane registry for dev context; adds fallback pane; adds new (external) pane imports. |
Comments suppressed due to low confidence (1)
test/unit/data-reformat-test.test.ts:138
it.skip(...)disables coverage for the Group label behavior. If groups are no longer meant to be label-able by this pane, update the test to assert the new expected output (likelynull) or remove it, rather than leaving it skipped.
it.skip('returns a good label Group if Group', () => {
const thing = sym(base + 'thing5')
store.add(thing, ns.rdf('type'), ns.vcard('Group'), doc)
expect(pane.label(thing, context)).toEqual('Group')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "css-loader": "^7.1.4", | ||
| "eslint": "^9.39.2", | ||
| "eslint-plugin-import": "^2.32.0", | ||
| "file-loader": "^6.2.0", |
There was a problem hiding this comment.
file-loader is added to devDependencies, but the webpack configs in this PR use Webpack 5 asset modules (type: 'asset/resource') and the repo doesn't reference file-loader anywhere else. Keeping an unused loader increases dependency surface area and lockfile size; either remove file-loader or switch the config to actually use it (one or the other).
| "file-loader": "^6.2.0", |
| // with the new design we only display Address Books | ||
| // individuals are rendered through the profile-pane but not Organizations |
There was a problem hiding this comment.
The comment says “we only display Address Books”, but the implementation still returns Contact for vcard:Organization and returns null for Individuals/Persons/Groups. Either the comment should be updated to match the intended behavior, or the label logic should be adjusted so it’s clear which subjects this pane is expected to advertise for.
| // with the new design we only display Address Books | |
| // individuals are rendered through the profile-pane but not Organizations | |
| // In the current design this pane advertises for Address Books and Organizations. | |
| // Individual/person/group resources are intentionally not advertised here; those | |
| // cases are handled elsewhere or remain disabled below. |
| import { default as contactsPane } from '../src/contactsPane' | ||
| import { default as profilePane } from '../../profile-pane/src/index' | ||
| import { longChatPane } from '../../chat-pane/src/longChatPane' |
There was a problem hiding this comment.
These dev-only imports use relative paths outside this repository (../../profile-pane/..., ../../chat-pane/...). In a standalone checkout of contacts-pane, npm run start / the dev webpack build will fail at compile time because those files don't exist. Consider replacing these with local stubs (like unsupportedPane) or optional/dynamic loading so the dev build remains self-contained.
| it.skip('returns a good label Contact for Individual', () => { | ||
| const thing = sym(base + 'thing2') | ||
| store.add(thing, ns.rdf('type'), ns.vcard('Individual'), doc) | ||
| expect(pane.label(thing, context)).toEqual('Contact') |
There was a problem hiding this comment.
it.skip(...) disables coverage for the Individual label behavior. If the pane is intentionally no longer label-able for Individuals, it's better to update the assertion to the new expected behavior (e.g. null) or remove the test with a brief comment explaining the deprecation, rather than skipping indefinitely.
| it.skip('returns a good label Contact for Individual', () => { | |
| const thing = sym(base + 'thing2') | |
| store.add(thing, ns.rdf('type'), ns.vcard('Individual'), doc) | |
| expect(pane.label(thing, context)).toEqual('Contact') | |
| it('returns null for Individual', () => { | |
| const thing = sym(base + 'thing2') | |
| store.add(thing, ns.rdf('type'), ns.vcard('Individual'), doc) | |
| expect(pane.label(thing, context)).toEqual(null) |
| it.skip('returns a good label Person if Person', () => { | ||
| const thing = sym(base + 'thing3') | ||
| store.add(thing, ns.rdf('type'), ns.schema('Person'), doc) | ||
| expect(pane.label(thing, context)).toEqual('Person') | ||
| }) |
There was a problem hiding this comment.
it.skip(...) disables coverage for the Person label behavior. If this is an intentional behavior change, update the test expectations to match the new contract instead of skipping the test.
| it.skip('returns a good label Person if foaf:Person', () => { | ||
| const thing = sym(base + 'thing4') | ||
| store.add(thing, ns.rdf('type'), ns.foaf('Person'), doc) | ||
| expect(pane.label(thing, context)).toEqual('Person') | ||
| }) |
There was a problem hiding this comment.
it.skip(...) disables coverage for the foaf:Person label behavior. If the label logic was intentionally changed, update the assertion to the new expected output instead of skipping.
| options.editable = kb.updater.editable(person.doc().uri, kb) | ||
| const div = dom.createElement('div') | ||
| div.classList.add('webidControl') | ||
| div.classList.add('webidControl', 'webidControl--mobile') |
There was a problem hiding this comment.
webidControl--mobile is added unconditionally. Because src/styles/webidControl.css applies many layout overrides when .webidControl.webidControl--mobile matches, this effectively forces the “mobile” layout even on wide/desktop screens (making the .contactPane--narrow gate irrelevant for those rules). If the intent is to only apply these overrides in narrow mode, only add this class conditionally (or remove it and rely on .contactPane--narrow .webidControl).
| div.classList.add('webidControl', 'webidControl--mobile') | |
| div.classList.add('webidControl') |
| display: none; | ||
| font-size: var(--font-size-base); /* new */ | ||
| line-height: 1; /* new */ | ||
| } | ||
|
|
||
| .skip-link:focus { | ||
| top: 6px; /* was top: 0; */ | ||
| display: none; /* in contact-pane this isn't there */ |
There was a problem hiding this comment.
The .skip-link utility is set to display: none, and it remains display: none on :focus. A skip link needs to be visually hidden off-screen (not display:none) so it can receive keyboard focus and become visible when focused. As written, this class prevents skip links from ever being reachable/visible, undermining the intended accessibility behavior.
| display: none; | |
| font-size: var(--font-size-base); /* new */ | |
| line-height: 1; /* new */ | |
| } | |
| .skip-link:focus { | |
| top: 6px; /* was top: 0; */ | |
| display: none; /* in contact-pane this isn't there */ | |
| font-size: var(--font-size-base); /* new */ | |
| line-height: 1; /* new */ | |
| } | |
| .skip-link:focus { | |
| top: 6px; /* was top: 0; */ |
| /* Utility helper classes */ | ||
| .hidden { | ||
| display: none !important; | ||
| } | ||
|
|
||
| .btn-primary { | ||
| min-height: var(--min-touch-target); | ||
| padding: var(--spacing-sm) var(--spacing-md); | ||
| border: 1px solid var(--color-primary); | ||
| border-radius: var(--border-radius-base); | ||
| background: var(--color-primary); | ||
| color: var(--color-background); | ||
| font-weight: 600; | ||
| cursor: pointer; | ||
| transition: all var(--animation-duration) ease; | ||
| } | ||
|
|
||
| .btn-primary:hover { | ||
| background: color-mix(in srgb, var(--color-primary) 90%, black); | ||
| box-shadow: var(--box-shadow-hover); | ||
| } | ||
|
|
||
| .btn-primary:active { | ||
| /* different from mashlib .btn-primary:active { | ||
| box-shadow: var(--box-shadow-active); | ||
| } | ||
|
|
||
| .btn-primary:disabled { | ||
| opacity: var(--opacity-disabled); | ||
| cursor: not-allowed; | ||
| transform: none; | ||
| } | ||
|
|
||
| .btn-secondary { | ||
| min-height: var(--min-touch-target); | ||
| padding: var(--spacing-sm) var(--spacing-md); | ||
| border: var(--border-width-thin) solid var(--color-secondary); | ||
| border-radius: var(--border-radius-base); | ||
| background: var(--color-secondary); | ||
| color: var(--color-background); | ||
| font-weight: var(--font-weight-bold); | ||
| cursor: pointer; | ||
| transition: all var(--animation-duration) ease; | ||
| text-decoration: none; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .btn-secondary:hover { | ||
| background: color-mix(in srgb, var(--color-secondary) 85%, black); | ||
| } | ||
|
|
||
| .btn-secondary:disabled { | ||
| opacity: var(--opacity-disabled); | ||
| cursor: not-allowed; | ||
| } | ||
|
|
||
| .action-button-focus:focus, | ||
| .action-button-focus:focus-visible { | ||
| outline: 3px solid var(--color-primary) !important; | ||
| outline-offset: 2px !important; | ||
| box-shadow: 0 0 0 2px var(--color-background), var(--box-shadow-focus) !important; | ||
| z-index: 1; | ||
| } | ||
| } */ |
There was a problem hiding this comment.
src/styles/utilities.css no longer defines the generic .hidden { display: none !important; } helper, but the runtime code relies on it in multiple places (e.g. toggling visibility in src/contactsPane.js, src/addressBookPresenter.js, and for the modal overlay in src/localUtils.js via className = 'focus-trap hidden'). Without a .hidden rule shipped with the component styles, elements intended to be hidden will remain visible (and the modal overlay may render immediately when created). Re-introduce the .hidden utility in this file (and keep it part of the distributed styles).
No description provided.