feat: Ui update to frappe ui v1.0.0 - #83
Conversation
1b457e0 bumped frappe-ui to 1.0.0-beta.24 in package.json but left yarn.lock resolving ^0.1.270 -> 0.1.278, so `yarn install --frozen-lockfile` failed and a fresh install produced a different tree than the one being tested — the components in that commit were adapted for frappe-ui 1.x, not 0.1.x. Re-resolving the lock also picks up the patched versions of the vulnerable transitive dependencies Dependabot flagged: brace-expansion 1.1.15 -> 1.1.18, 5.0.6 -> 5.0.9 (DoS: exponential expansion, unbounded expansion OOM) js-yaml 4.2.0 -> 4.3.1 (quadratic CPU via merge-key chains) postcss 8.5.15 -> 8.5.25 (path traversal via sourceMappingURL) dompurify 3.4.11 -> 3.4.13 (CUSTOM_ELEMENT_HANDLING bypass) echarts 5.6.0 -> 6.1.0 (XSS; only 6.x is patched while frappe-ui still declares ^5.6.0 — hence a resolution) echarts is not part of the built bundle (its size is byte-identical before and after), so pinning a major carries no runtime risk here.
crm 1.81 rebuilt its sidebar on frappe-ui's Sidebar/SidebarItem components: a row is now `div[data-slot="sidebar-item"]` (h-7) wrapping a link whose icon sits in a `span.grid`, and collapsing sets an inline width instead of toggling `w-12`. Our injection hardcoded the crm <= 1.74 shape (`button > div.px-2.py-[7px]`), so on 1.81 the anchor lookup — which required a `<button>` — matched nothing and no custom items were injected at all. Rather than re-copy their markup every release, rows are now built by cloning a real row out of the CRM's own sidebar and swapping icon + label (utils/sidebarRow.js). Height, padding, hover, focus ring, dark mode and collapse behaviour then come from whatever version is installed. Group headers likewise clone the CRM's own `div[data-slot="sidebar-label"]`, chevron included. Fixes found by diffing our rendered rows against the CRM's: - The group header sat 8px right of "Public Views": syncCollapse() treated every label like a nav row and added `ml-2`, but a section label only fades its <h3> (`w-auto opacity-100`, no margin). The tag now records which animation applies. - Collapsed rail: icons stayed left-hugged, because the host swaps `pl-2` for `justify-center` and grows the icon holder to `size-7` — something static clones never did. The header also collapsed to an empty 28px gap (frappe-ui shows a divider rule there instead), and group children were force-hidden, so a group's icons vanished from the rail entirely. - Reloading while collapsed showed icons with no labels after expanding: the cloned template was already in rail form (`ml-0 w-0 opacity-0`), leaving nothing to restore. Clones are now normalised to the expanded state first. - Labels render through `__()`, so matching the literal "Call Logs" broke the whole injection on a translated site. There is now a structural fallback, and the label element is located by layout rather than by the template's text. - The Search row showed a list icon: `search` was missing from the curated icon map. List view: the route host div used `height: 100%` inside FCRM's `flex-1 flex flex-col h-full overflow-auto` column — one AppHeader too tall, so the column scrolled and the bottom of the list was cut off. It now sizes as a flex child. Its header also drew a second bar below the CRM's empty `#app-header` slot; it teleports into that slot like FCRM's own pages do. Also dead code and robustness cleanups the review turned up: the unreachable crm <= 1.74 markup branch is gone (the caller already requires a template), and the cached sidebar element is re-resolved when the SPA detaches it. Verified against the markup both versions actually render — 67 assertions over happy-dom fixtures for crm 1.74 rows, crm 1.81 rows, a collapsed sidebar and a translated sidebar.
…nerations fb3b61c's aliases were written from frappe-ui's legacy `tailwind/colors.js`, which is not what the shipped preset emits. Two wrong premises made most of those rules dead: - ink variables are `--ink-*` in both generations, not `--text-ink-*`; - both generations store whole colours (0.1.x `#fff`, 1.x `oklch(...)`), so wrapping them in `rgb()` produced invalid values, not RGB channels. Regenerated against the stylesheets crm 1.74.1 (frappe-ui 0.1.261) and crm 1.81.0 (frappe-ui 1.0.0-beta.29) actually produce. Each rule now names the 1.x token first and falls back to its 0.1.x counterpart, then to a literal, so one chain serves either host; because the variables are the host's own, dark mode and any theme customisation follow automatically and no duplicate dark-mode rules are needed. Coverage also extends past the pure renames to the ramps the same migration renumbered (surface-gray-5 -> 8, ink-red-2 -> 5, outline-gray-5 -> 7, ...), which is what the first attempt missed: those class names exist only in 1.x, so on a 0.1.x host our solid buttons, hover states and error text had no rule at all. Every selector stays wrapped in `:where()` (zero specificity), so wherever the host ships the class its own rule wins — these can only fill gaps, never restyle the CRM. Result: of the 97 token classes the bundle uses, 0 are unstyled on either version (was 49 unstyled on 1.74.1 and 27 on the 1.x line).
- public/js/search_bar.js: the pre-Vue Cmd+K implementation from c8c1f44, superseded by SearchDialog.vue. Nothing references it — no hooks include, no entry in www/crm.py's EXTENSION_SCRIPTS, no mention in the README. - public/js/style.css: a stale build artifact (frappe-ui popover keyframes from an old bundle CSS). b5098f5 stopped tracking Vite outputs but missed this one; the live file is the gitignored frappe-crm-xt-frontend.css. - frontend/pnpm-workspace.yaml: vestigial since ed8b0c5 moved to yarn classic. It only held unanswered `allowBuilds:` placeholders and there is no pnpm-lock.yaml. - @rollup/rollup-darwin-arm64 optionalDependency: never imported, and it pinned a darwin-arm64 binary into a lockfile that CI resolves on Linux. Rollup declares its own per-platform optional deps, so the correct binary is still installed. Also ignore .ruff_cache, which was untracked but unignored (invisible so far only because the directory happens to be empty). Verified: the six patched dependency versions are unchanged after re-resolving, and the bundle still builds.
# Conflicts: # frontend/yarn.lock
There was a problem hiding this comment.
Pull request overview
Updates the extension for frappe-ui 1.0 compatibility, primarily modernizing styling and sidebar integration.
Changes:
- Upgrades frontend dependencies and design tokens.
- Reworks injected sidebar rows using native CRM markup.
- Updates supporting metadata, assets, and custom fields.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/copilot-code-review.yml |
Adds review setup workflow. |
.gitignore |
Ignores Ruff cache. |
pyproject.toml |
Revises Frappe/CRM compatibility ranges. |
frontend/package.json |
Upgrades frappe-ui dependencies. |
frontend/yarn.lock |
Locks upgraded dependency tree. |
frontend/pnpm-workspace.yaml |
Removes pnpm build settings. |
frontend/src/utils/sidebarRow.js |
Adds native sidebar-row cloning utilities. |
frontend/src/styles/bundle.css |
Adds cross-version design-token aliases. |
frontend/src/lucideIcons.js |
Adds sidebar icons. |
frontend/src/components/App.vue |
Integrates cloned sidebar rows and collapse behavior. |
frontend/src/components/SearchDialog.vue |
Updates surface tokens and spacing. |
frontend/src/components/ListFilterLocal.vue |
Updates popover and typography tokens. |
frontend/src/components/InjectedEventsTab.vue |
Modernizes event-tab styling. |
frontend/src/components/InjectedEventModal.vue |
Modernizes modal styling. |
frontend/src/components/ExtListView.vue |
Teleports list headers and updates styling. |
frontend/src/components/ExtListRows.vue |
Updates typography utility. |
frontend/src/components/EventNotifications.vue |
Updates notification surface token. |
frontend/src/components/Attendee.vue |
Updates combobox styling. |
frappe_crm_xt/setup/custom_fields.json |
Removes filters and descriptions. |
frappe_crm_xt/public/js/style.css |
Removes obsolete generated CSS. |
frappe_crm_xt/public/js/search_bar.js |
Removes legacy search implementation. |
frappe_crm_xt/__init__.py |
Bumps version to 1.1.0. |
Suppressed comments (2)
frontend/src/utils/sidebarRow.js:156
- Removing
hrefmakes cloned anchors non-focusable, whilerole="button"does not add native keyboard behavior. In CRM versions whose sidebar row contains an anchor, these custom rows can no longer be reached or activated by keyboard; add a tab stop and Enter/Space activation.
row.querySelectorAll('a').forEach((a) => {
a.removeAttribute('href')
a.removeAttribute('target')
a.setAttribute('role', 'button')
})
frontend/src/utils/sidebarRow.js:204
- The cloned modern section label is a clickable
div, but it has neither button semantics nor keyboard activation. Consequently the new collapsible groups cannot be toggled without a pointer; expose the header as a keyboard-operable button.
const header = template.cloneNode(true)
header.removeAttribute('id')
header.classList.add('crm-xt', 'cursor-pointer', 'select-none')
header.setAttribute('aria-label', label)
header.setAttribute('aria-expanded', 'false')
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e synchronization
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (4)
frontend/package.json:32
- This resolution forces ECharts 6.1.0 into
frappe-ui@1.0.0-beta.24, even though that package explicitly declaresecharts: ^5.6.0; the resulting lockfile confirms the incompatible major override. Remove the override, or upgrade to a frappe-ui release that explicitly supports ECharts 6 before forcing the new major.
"echarts": "6.1.0"
frappe_crm_xt/setup/custom_fields.json:448
- Restoring this static filter is required for Desk and other clients that do not execute the CRM form script. The form-script fixture explicitly relies on these static authorized/push-enabled conditions; without them, users can select a calendar that cannot receive events and calendar sync subsequently fails.
"insert_after": "custom_event_participants",
"depends_on": "eval:doc.custom_sync_with_calendar"
frontend/src/utils/sidebarRow.js:118
- Removing
hrefturns a cloned Frappe UI sidebar link into a non-focusable anchor. Since activation is wired only through a click listener, keyboard users can no longer reach or activate custom sidebar entries. Preserve button keyboard semantics by adding a tab stop and handling Enter/Space.
row.querySelectorAll('a').forEach((a) => {
a.removeAttribute('href')
a.removeAttribute('target')
a.setAttribute('role', 'button')
})
frontend/src/utils/sidebarRow.js:153
- In frappe-ui 1.0.0-beta.24,
sidebar-labelis a plain<div>. This clone is subsequently used as a clickable expand/collapse control, butaria-expandedalone does not make it focusable or keyboard-operable. Give it button semantics and activate it on Enter/Space.
header.classList.add('crm-xt', 'cursor-pointer', 'select-none')
header.setAttribute('aria-label', label)
header.setAttribute('aria-expanded', 'false')
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (7)
frontend/package.json:33
frappe-ui@1.0.0-beta.24declaresecharts@^5.6.0, but this resolution forces it to use the incompatible 6.x major (as reflected by the combined lockfile entry). That bypasses the dependency's tested range and exposes Frappe UI charts to ECharts 6 behavior/default changes. Remove this override, or upgrade to a Frappe UI release that explicitly supports ECharts 6.
"resolutions": {
"echarts": "6.1.0"
}
frappe_crm_xt/setup/custom_fields.json:448
- Removing the static filter leaves non-CRM clients and direct API writes able to select any
Google Calendar. The updated form script claims this is enforced server-side, butdoc_events/task.pyonly checks that the field is present, and calendar sync then saves an Event with that calendar usingignore_permissions=True. Validate that the selected calendar is push-enabled/authorized and belongs to the current user before syncing; client-side filtering alone cannot enforce this.
"options": "Google Calendar",
"insert_after": "custom_event_participants",
"depends_on": "eval:doc.custom_sync_with_calendar"
frontend/src/utils/sidebarRow.js:136
- The Frappe UI 1.0 sidebar route template contains an
<a>. Removing itshrefmakes the cloned control unfocusable, androle="button"does not add Enter/Space activation, so keyboard users cannot open any cloned search or custom-sidebar row. Keep it in the tab order and handle button keyboard activation (or replace it with a real<button>).
row.querySelectorAll('a').forEach((a) => {
a.removeAttribute('href')
a.removeAttribute('target')
a.setAttribute('role', 'button')
frontend/src/utils/sidebarRow.js:172
- In Frappe UI 1.0,
data-slot="sidebar-label"is a non-interactive<div>. This clone is later used as a clickable group toggle, but adding cursor styling andaria-expandeddoes not make it focusable or keyboard-operable. Give non-interactive clones button semantics,tabindex="0", and Enter/Space handling, or render a real button.
header.classList.add('crm-xt', 'cursor-pointer', 'select-none')
header.setAttribute('aria-label', label)
header.setAttribute('aria-expanded', 'false')
frontend/src/components/App.vue:327
- On the target Frappe UI version, the native sidebar label contains only label/divider markup, so this branch never renders
icon. BecausefindNativeSectionLabel()succeeds in the upgraded UI, every configured group icon—including this app'slayout-gridicon—is silently ignored despiteiconbeing part of the documentedcrm_sidebarcontract. Pass the icon into the cloned label and render it, or keep using row-style markup for icon-bearing group headers.
const icon = item.icon || 'folder'
const nativeLabel = findNativeSectionLabel(document)
let headerBtn
if (nativeLabel) {
headerBtn = cloneNativeSectionLabel(nativeLabel, { label: item.label })
frappe_crm_xt/setup/custom_fields.json:286
- Deleting
link_filtersfrom this fixture does not clear it on upgraded sites.install_custom_fields()callscreate_custom_fields(..., update=True), which merges only keys present in each definition, so existing installations retain the old@rtcamp.comrestriction while fresh installations do not. Explicitly set this property to null (or add a migration) so upgrades converge to the new definition.
"options": "User",
"insert_after": "custom_project_type"
frappe_crm_xt/setup/custom_fields.json:447
- This omission also fails to remove the old filter during migration:
create_custom_fields(..., update=True)only updates supplied keys. Existing sites therefore keep queryingcustom_is_google_calendar_authorized—the optional field this change is trying to stop depending on—while fresh installs behave differently. Supply a portable replacement explicitly (or clear it in a migration).
"options": "Google Calendar",
"insert_after": "custom_event_participants",
dpk404
left a comment
There was a problem hiding this comment.
pyproject.tomllowers the frappe floor from>=16.0.0-devto>=15.0.0, a new claim of Frappe 15 compatibility.resolutionsforces echarts 6.1.0 where frappe-ui declares^5.6.0, a major version outside the declared range. The bundle imports no chart components, so it only affects audit output.- The title says frappe-ui v1.0.0;
package.jsonpins1.0.0-beta.24. - The PR description is empty.
…e accessibility in sidebar row handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
frontend/src/utils/sidebarRow.js:152
findNativeRowcan return the<a>or<button>itself, but these descendant-only selectors skip that root. In that path, the clone retains the nativehref/aria-currentand is not marked for collapse syncing, so middle-click can open the template route and the custom row can remain styled as current. Include the matching root in these lookups.
row
.querySelectorAll('[aria-current]')
.forEach((el) => el.removeAttribute('aria-current'))
row.querySelectorAll('a').forEach((a) => {
a.removeAttribute('href')
frontend/src/utils/sidebarRow.js:183
- The Frappe UI
SidebarLabelmatched here is a plain<div>, so the cloned group header is clickable but cannot receive focus or be toggled by keyboard. Add button semantics and Enter/Space activation when the cloned root is not already interactive.
header.classList.add('crm-xt', 'cursor-pointer', 'select-none')
header.setAttribute('aria-label', label)
header.setAttribute('aria-expanded', 'false')
frontend/src/components/App.vue:207
- Frappe UI v1
SidebarItemdeliberately keepspl-2and the icon's natural size while collapsed. These unconditional toggles instead center and enlarge only the injected clones, so they no longer align with adjacent native rows during the sidebar animation. Apply the legacy transform only to rows that are not modern[data-slot="sidebar-item"]clones.
document.querySelectorAll('[data-xt-link]').forEach((el) => {
el.classList.toggle('pl-2', !collapsed)
el.classList.toggle('justify-center', collapsed)
})
document.querySelectorAll('[data-xt-icon]').forEach((el) => {
el.classList.toggle('size-7', collapsed)
frappe_crm_xt/setup/custom_fields.json:448
- Removing the static filter lets Desk users select calendars with
push_to_google_calendar = 0; the CRM form script cannot protect Desk and explicitly states that this field should retain a static push-enabled filter. Such a selection passes server validation but will not be pushed, despite calendar sync being enabled. Keep the portable push-enabled filter while dropping only the optional authorization field.
"options": "Google Calendar",
"insert_after": "custom_event_participants",
"depends_on": "eval:doc.custom_sync_with_calendar"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
frappe_crm_xt/setup/custom_fields.json:448
- The updated form script says Desk is still constrained by a static
push_to_google_calendarfilter, but this field now has nolink_filters. Views that do not execute the CRM form script can therefore select calendars with Push disabled, despite calendar sync requiring a push-enabled calendar. Keep the portable push filter here while leaving the optional authorization field out.
"fieldname": "custom_google_calendar_link",
"fieldtype": "Link",
"label": "Google Calendar",
"options": "Google Calendar",
"insert_after": "custom_event_participants",
"depends_on": "eval:doc.custom_sync_with_calendar"
The merge-base changed after approval.
# Conflicts: # frontend/yarn.lock
This pull request includes a version bump and clarifies how icons are handled in the CRM sidebar configuration. The main updates are to the documentation, specifying that sidebar group headers do not support custom icons and updating the package version.
Documentation updates:
README.mdto clarify that theiconproperty is not supported for sidebar items of typegroup, and that FCRM section headers use a chevron and text only.iconproperty from the "Procurement" group example in the CRM sidebar configuration inREADME.mdto match the documentation.Version update:
frappe_crm_xt/__init__.pyfrom1.1.1to1.2.0.