[full-ci]update button appearance in folder view modal#13581
[full-ci]update button appearance in folder view modal#13581PrajwolAmatya wants to merge 1 commit into
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
Signed-off-by: Prajwol Amatya <prajwolamatya11@gmail.com>
34033f0 to
ce1dc66
Compare
LukasHirt
left a comment
There was a problem hiding this comment.
This would unfortunately break behaviour in other modals. The button is passive on purpose to differentiate between the confirm (primary) and cancel (passive) buttons. I guess if we would need to solve it specifically for the folder modal, we would need to introduce a prop which can overwrite the button variant.
DeepDiver1975
left a comment
There was a problem hiding this comment.
Thanks for tackling the WCAG color-contrast violation on the modal cancel button — the underlying problem (the passive/outline Close folder button failing the 4.5:1 contrast ratio against the dark modal background) is real and worth fixing. A few blocking concerns before this can go in, though.
1. Snapshot tests are stale → CI is red (blocking).
The change only edits OcModal.vue, but OcModal is the shared design-system modal and its committed snapshot still encodes the old attributes:
packages/design-system/src/components/OcModal/__snapshots__/OcModal.spec.ts.snap still has variation="passive" appearance="outline" and oc-button-passive oc-button-passive-outline on .oc-modal-body-actions-cancel. The snapshot test will mismatch — which matches the failing continuous-integration/drone/pr build. Please regenerate snapshots (pnpm --filter design-system test -u or the repo's equivalent) and commit them so CI goes green.
2. Design-system regression: two primary/filled buttons (blocking concern).
This change is not scoped to the password-protected folder modal — it changes the default cancel button for every modal in the app. The confirm button already defaults to variation="primary" / appearance="filled" (buttonConfirmAppearance = ref('filled')). With this PR the cancel button also becomes primary + filled, so every modal now renders two identical filled-primary buttons side by side. That defeats the visual hierarchy (a dialog should have a single emphasized primary action and a de-emphasized dismiss action) and contradicts the component's own documented contract ("the cancel buttons defaults to the passive variation"). Making cancel co-equal with confirm is a UX regression beyond the reported issue.
A more targeted fix that preserves the secondary-button semantics while meeting contrast would be preferable, e.g. keep the cancel button passive but use appearance="filled" (filled passive), or fix the passive/outline token contrast in the theme, rather than promoting cancel to primary/filled globally. Please confirm with design which direction is intended, and ideally validate visually across the common modals (delete/confirm dialogs, not just this one).
3. Missing changelog fragment.
This is a user-facing UI/accessibility change, so it needs a changelog/unreleased/ fragment following changelog/TEMPLATE (e.g. Bugfix: ... with the issue/PR URLs). There's no linked issue in the description (Fixes <issue_link> is still a placeholder) — please link the accessibility issue and reference it in the fragment, or use the PR number if there is genuinely no issue.
4. PR metadata. The "Related Issue" and "Types of changes" sections are still the unfilled template. Please link the issue and tick the bug-fix box.
Summary: the accessibility goal is valid, but the current approach has stale snapshots (red CI), an app-wide double-primary-button regression, and a missing changelog. Requesting changes.
Description
The
Close Folderbutton in password-protected folder modal is passive, which violates the minimum color contrast threshold.Accessibility test returns following error:
Updated the button attributes to fix the color contrast threshold.

Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Open tasks: