[Cohort builder] Share option is not available in filter card button section - #3010
Open
khairul-syazwan wants to merge 14 commits into
Open
[Cohort builder] Share option is not available in filter card button section#3010khairul-syazwan wants to merge 14 commits into
khairul-syazwan wants to merge 14 commits into
Conversation
Relocate the 'Allow sharing' checkbox from the save-bookmark dialog into the FiltersFooter bar so it is always visible for shareable cohorts. Toggling it now marks the cohort dirty and enables Save, addressing the complaint that the share option only appeared after changing filter configuration. Refs #2878
Move the "Allow sharing" checkbox out of the footer button bar into its own 40px row at the bottom of the filter side panel, directly above the action buttons, per Figma node 1810-230545. The row carries an info icon whose tooltip explains the sharing behaviour, and the filter scroll area shrinks by the extra row height only when the row is rendered. A cohort that has been saved before now saves straight away with the existing success toast instead of opening an empty confirm dialog; the name dialog is kept only for flows that create a new record. Refs #2878
… row The custom appCheckbox (2px navy 24px box) and the UI5 glyph info icon did not match the Figma mockup, which uses MUI-style controls. Swap them for the Vuetify components the app is migrating to: - v-checkbox (compact, hide-details) styled to the Figma row: 18px outline box 8px from the panel edge, Body 1 label, no helper-text gutter. - v-icon mdi-information-outline for the circled-i affordance. - v-tooltip replacing the native title attribute, styled to the Figma tooltip tokens: white surface, Neutral/Default text, Caption 1, 12px padding, 4px radius, 222px max width. Vuetify 3.12 is already registered for both entrypoints and declares VCheckbox/VTooltip defaults in src/plugins/vuetify.ts. Refs #2878
Bootstrap gives every `label` a 0.5rem bottom margin, which pushed the Vuetify checkbox label 4px above the vertical centre of the box and the info icon. Reset the margin so all three share one centre line. Also stop overriding the label colour: the inherited near-black body text (#212529) is what the Figma mockup draws, whereas --color-ui-dark-text resolves to the navy heading colour. Refs #2878
khairul-syazwan
requested review from
LSriragavan,
brandantck,
jerome-ng,
maggie-li-yd and
p-hoffmann
as code owners
July 28, 2026 07:06
khairul-syazwan
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Moves the “Allow sharing” control out of the save dialog and into the cohort filter side panel footer so it participates in dirty-checking and aligns with the updated Figma design.
Changes:
- Added a dedicated “Allow sharing” row above the footer action bar and wired it into
hasChangesso toggling it enables Save. - Adjusted save flow to skip the name dialog when saving an already-owned cohort.
- Updated styling (Vuetify checkbox/tooltip + layout offsets) and added i18n tooltip copy.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plugins/ui/apps/vue-mri-ui-lib/src/styles/filtersfooter.scss | Adds share row + Vuetify checkbox/tooltip styling and introduces global tooltip override. |
| plugins/ui/apps/vue-mri-ui-lib/src/styles/filters.scss | Adjusts content bottom offset when the share row is present. |
| plugins/ui/apps/vue-mri-ui-lib/src/lib/i18n.ts | Adds a new tooltip i18n key for the share info tooltip (en/de/zh). |
| plugins/ui/apps/vue-mri-ui-lib/src/components/FiltersFooter.vue | Renders share checkbox + tooltip in the footer, updates dirty-check logic, and refines save behavior. |
Comments suppressed due to low confidence (1)
plugins/ui/apps/vue-mri-ui-lib/src/styles/filtersfooter.scss:1
- This styling depends on Vuetify’s internal class structure (
.v-selection-control__wrapper,.v-selection-control__input,.v-selection-control--dirty, etc.). That’s brittle across Vuetify upgrades and can lead to silent UI regressions. Where possible, prefer Vuetify-supported styling hooks (component props, density/size APIs, theme variables, or documented CSS variables) and keep the internal class overrides to the minimum required surface area.
// Height of the optional "Allow sharing" row. Kept in sync with the
maggie-li-yd
approved these changes
Jul 29, 2026
Moving the allow-sharing checkbox into the filter card footer adds a row
above Add Filters / Save, which shifts every baseline that captures the
Cohorts left panel. The first spec (01-example) already fails on a 460px
diff and 68 tests never ran behind it, so the full extent is unknown -
regenerate the whole tree and diff.
Three parts, all temporary:
* package.json: playwright test -> playwright test -u
* docker-build-push.yaml: bind-mount tests/ into the e2e container, so
-u writes into the workspace instead of the --rm container FS
* docker-build-push.yaml: upload the regenerated *-snapshots as an
artifact, since the existing upload only covers test-results/
Revert all three once the baselines are committed. While -u is set the
visual assertions are vacuous and e2e is green without validating
anything - this must not merge in this state.
The new 'Allow sharing' label makes page.getByText('All') ambiguous -
getByText is substring-matching, so 'All' now also matches 'Allow sharing'
and Playwright fails with a strict mode violation:
strict mode violation: getByText('All') resolved to 2 elements:
1) <span class="multiselect__placeholder">All</span>
2) <label ...> aka getByText('Allow sharing')
Scope the three unscoped call sites to the Gender filter card, matching the
pattern already used in inclusion-report.spec.ts:37. The remaining sites
were already scoped to a parent locator and are unaffected.
e2e_PA_and_cohorts.spec.ts:150 (.first()) and filtering-barchart.spec.ts:101
(.nth(2)) are index-based rather than strict, and the allow-sharing row
renders in the filter panel footer - after the filter cards in DOM order -
so their indices should be unchanged. Left as-is deliberately.
Two behaviour changes in this PR break e2e specs that -u cannot mask:
1. The allow-sharing checkbox moved out of the save dialog into the filter
card footer. Specs clicked it *after* opening the dialog, where it no
longer exists and the modal overlay would intercept the click. Moved the
click before the Save click and switched to the new
data-testid=pa-share-cohort-checkbox hook.
- cohort-generation.spec.ts
- patient-group_comparison.spec.ts (createCohortWithOneConditionOccurrenceFilercard)
2. FiltersFooter.openSaveBookmark() now only opens the naming dialog when
needsSaveDialog (isNewCohort || isNotUserSharedBookmark) is true, so
re-saving an already-saved, user-owned cohort saves straight away via
cmd:'update' and reports through the MRI_PA_UPDATE_BMK_SUCCESS toast.
Dropped the now-unreachable footer Save click on those re-saves.
- bookmark.spec.ts:186 (CI-confirmed: 20s click timeout)
- patient-group_comparison.spec.ts x2
- cohort-generation.spec.ts
The remaining footer Save call sites all follow a 'D2E' new-cohort click, so
they still get the dialog and are left unchanged.
…ckbox
Follow-up sweep on every string and class the old control used (title,
label, row-checkbox, appCheckbox, SHARED_BOOKMARK), not just the label text
matched previously. Three sites remained, only one of which CI had reached
before stopping at maxFailures=1:
* bookmark.spec.ts:313 - CI-confirmed 20s click timeout on
getByTitle('Allow bookmark to be visible'),
which was MRI_PA_BMK_SHARED_BOOKMARK_TITLE on
the removed dialog checkbox
* patient-list.spec.ts:74 - same pattern, had not run yet
* patient-analytics-config-mri.spec.ts - asserted '- text: Allow sharing' in
the dialog aria snapshot and clicked
.app-checkbox-container, the appCheckbox this
PR removes entirely
All three now toggle sharing through data-testid=pa-share-cohort-checkbox
before the save dialog opens, and the stale line is dropped from the aria
snapshot.
…cohorts The previous sweep grepped 'appcheckbox' and so missed '.app-checkbox-container' - the hyphens meant the pattern never matched. That class comes from lib/ui/app-checkbox.vue, which FiltersFooter no longer imports, so both clicks timed out once the run got past bookmark.spec.ts. Both are new-cohort saves, so the dialog still opens; only the sharing toggle moved. Set it via data-testid=pa-share-cohort-checkbox before opening the dialog. Cross-checked the whole suite against every class, attribute and i18n key this PR removes (app-checkbox, row-checkbox, filters-footer, MRI_PA_BMK_SHARED_ BOOKMARK_TITLE/TEXT): no references remain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2878
Problem
The "Allow sharing" option only existed inside the save-bookmark dialog, so it was invisible until the user had already committed to saving — and because it didn't participate in the dirty check, Save stayed disabled until some unrelated filter configuration changed. A user who only wanted to share an existing cohort had no way to enable Save.
Changes
FiltersFooter.vue.filters-footeris now two rows: a 40px.filters-footer__sharerow above the existing.filters-footer__actionsbar (reset | Add filters | Save). The checkbox is no longer inside the dialog, and is not inline beside Save.hasChangesincludesshareChanged = canShare && shareBookmark !== !!getActiveBookmark?.shared, so toggling "Allow sharing" alone enables Save — the core complaint in the issue.openSaveBookmark()only opens the name dialog when a new record is created (needsSaveDialog = isNewCohort || isNotUserSharedBookmark). An already-saved cohort owned by the current user now saves directly, reporting through the existing success toast, instead of showing a confirm dialog whose body was empty.Vuetify components. The mockup uses MUI-style controls, and this app is migrating to Vuetify, so the legacy controls were swapped for the Vuetify equivalents. Vuetify 3.12 was already registered for both entrypoints (
main.tsand the single-spalifecycles.ts) and already declaresVCheckbox/VTooltipdefaults insrc/plugins/vuetify.ts, so no wiring was added:appCheckbox(2px navy 24px box)v-checkbox(density="compact",hide-details)appIcon icon="information"(UI5 glyph, boxedi)v-icon icon="mdi-information-outline"titleattributev-tooltipfiltersfooter.scss— row/action-bar split; Vuetify checkbox trimmed to the Figma row (18px outline box 8px from the panel edge,rgba(0,0,0,0.6)unchecked, primary when checked, Body 1 label); tooltip styled to the Figma tokens (white surface,#595757text, Caption 1 / 12px, 12px padding, 4px radius, 222px max-width).filters.scss—&:has(.filters-footer__share) .filters-content { bottom: 104px }. The offset applies only when the row renders, because the row is gated behindcanShare(theadminOnlySharingfeature flag); a fixed offset would leave a dead gap when sharing is disallowed.i18n.ts— newMRI_PA_BMK_SHARED_BOOKMARK_TOOLTIP(en/de/zh) for the tooltip copy. The existing_TEXT/_TITLEkeys are untouched becauseQueryFilterModern.vuealso consumes them.Two styling gotchas worth knowing
Both were found by measuring the rendered DOM, and both will bite the next Vuetify control added to this app:
label { margin-bottom: 0.5rem }applies to Vuetify's.v-label, pushing checkbox labels 4px above the control's centre line. The label needs an explicitmargin-bottom: 0.--color-ui-dark-textis the navy heading colour (#000080), not body text. The near-black the mockups draw is the inherited#212529.The tooltip rule is also deliberately top-level and prefixed
.v-tooltip > .v-overlay__content— Vuetify teleports overlay content to the document root, and its own dark-surface rule outranks a plaincontent-class.Verification
Per repo convention, rendered UI behaviour is verified with Playwright rather than component-mounting unit tests.
Playwright: 20/20 passed against a local portal. The script creates its own cohort and deletes it afterwards, so it is repeatable.
bottom: 104pxwith no overlap of the rowrgb(33, 37, 41)mdi-information-outlinecolor rgb(89,87,87),background rgb(255,255,255),font-size 12px,width 222pxUnit tests: 643 passed. One pre-existing, unrelated failure —
src/lib/ui/__tests__/app-segmented-button.test.ts("updates internal state when value prop changes"), a stale assertion that also fails ondevelop.Test plan
adminOnlySharingrestricting sharing → row hidden, and filter cards still scroll to the top of the action bar with no gap