[DREAM-752] Links in collapsible headers collapse/expand before redirecting - #501
Conversation
69b83fe to
a99c59a
Compare
a99c59a to
9373085
Compare
🦋 Changeset detectedLatest commit: 490a39e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There was a problem hiding this comment.
Pull request overview
This PR addresses DREAM-752 by preventing collapsible headers (OpenProject CollapsibleSection/CollapsibleElement) from toggling collapse/expand when users interact with nested links/buttons, so navigation/actions aren’t disrupted by the collapse behavior.
Changes:
- Update the shared
CollapsibleElement#togglebehavior to ignore clicks originating from links/buttons. - Add a hidden Lookbook preview demonstrating a link inside the collapsible title.
- Add a system test ensuring clicking a title link does not collapse the section, plus a changeset entry.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/components/primer/open_project/collapsible.ts |
Adds event-target filtering to prevent nested interactive elements from triggering collapse. |
test/system/open_project/collapsible_section_test.rb |
Adds a regression test for clicking a link inside the collapsible title area. |
previews/primer/open_project/collapsible_section_preview.rb |
Registers a hidden preview scenario for a link-in-title case. |
previews/primer/open_project/collapsible_section_preview/with_link_in_title.html.erb |
Implements the preview template rendering a link in the title slot. |
.changeset/orange-insects-rest.md |
Documents the patch-level behavioral change for release notes. |
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
Unfortunately I haven't been able to test manually. There is an issue with my local dev environment + transpiling TypeScript: it might be that rebasing resolves things.
That said, the Copilot comments look correct to me. As far as I can tell, the current implementation still toggles the section when a link is followed using the keyboard.
I would still prefer an alternative, accessible approach that avoids nested interactive elements entirely.
Regarding the downstream use in BorderBoxListComponent:
- I'd prefer to make
CollapsibleHeaderprivate to core, or inline it there. Its only other consumer is in meetings, which could ideally be migrated toBorderBoxListComponentover time. - I don't think
CollapsibleSectionneeds to support links, so I'm not sure it's useful to add tests for that behaviour at the component level.
PR #511 should help quieten the snapshot noise.
e7d2ba2 to
e920b7e
Compare
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
I suspect we might need to revisit the increased spacing once we have integrated this downstream. For now though, this looks good! 👍🏻
|
@HDinger this could do with a rebase prior to merging! |
… the collapse mechanism
…at to fulfill accessibility requirements
ea42c11 to
f97584e
Compare
What are you trying to accomplish?
Avoid that clicks on links or button in a collapsible section trigger the collapse mechanism
List the issues that this change affects.
https://community.openproject.org/wp/DREAM-752
Risk Assessment