nextcloud / nextcloud/office

Adopt Vue composables for stateful non-rendering logic, starting with TemplateSection's scroll-arrows

Open
#134 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

0. Needs triage enhancement
Dominant language
JavaScript
Stars
6
Forks
1
Avg merge
1d 3h
Merged PRs (30d)
10

Description

TL;DR: This app has no composables yet — stateful logic that doesn't render anything (dialog state, view-mode persistence, scroll-arrow tracking) currently lives inline in <script setup> blocks alongside markup-driving state. Two PRs, not one: a proof-of-concept extraction first, then — once that's proven out and the conventions hold up in practice — a second PR that deliberately extends the same model to the rest of the identified clusters. Not a single big-bang PR covering everything at once, and not left to happen opportunistically one at a time either.

Detail
Why now

Found while investigating a recurring test-coverage gap across #126/#127/#130 (tracked separately in #133): the same file keeps absorbing new interactive elements and derived state without ever being split up. Composables are the piece "extract it" doesn't cover on its own — component extraction splits render output, composables split stateful logic with no direct template dependency. This project is small (5 Vue files, ~1000 lines total) and young, so establishing the convention now is cheap; it gets more expensive the more code lands on the current shape.

PR 1 — proof of concept: TemplateSection.vue's scroll-arrows cluster

list (element ref) + canScrollLeft/canScrollRight + updateArrows() + scrollByStep() + the onMounted/onUnmounted wiring is a self-contained "scrollable region with arrow buttons" state machine — reusable, no dependency on other in-flight work, and the same region already has an open a11y gap (#59: no aria-label identifying it as a scrollable region) worth closing while in there.

Check @vueuse/core before hand-rolling this one. Verified directly against its shipped type declarations: useScroll(element) already returns arrivedState.{left,right,top,bottom} (the inverse of canScrollLeft/canScrollRight) plus reactive x/y, and handles the listener attach/detach and resize-observation internally. The extraction should be a thin wrapper over useScroll, not new hand-rolled logic — this is the same "reuse before you write" ladder AGENTS.md already applies to @nextcloud/* packages and @nextcloud/vue components, extended to general-purpose Vue utilities. @vueuse/core@14.4.0's only peer dependency is vue@^3.5.0, which this app already satisfies.

Conventions to establish with this PR, not invent per-composable later:

  1. Check @vueuse/core first. Before hand-rolling a composable, check whether it already exists there (as with useScroll above). Prefer it the same way an existing @nextcloud/vue component or design token is preferred over a hand-rolled equivalent.
  2. Return individual refs, not a reactive() object. A composable that returns reactive({...}) breaks reactivity silently if the caller destructures it. Return individual refs (or toRefs() on an internal reactive object) so destructuring is always safe.
  3. A shared test helper for composables using lifecycle hooks. onMounted/onUnmounted throw outside an active component instance, so testing needs a small reusable helper — a trivial host component whose setup() just returns the composable — living in src/test-utils/, written once here rather than reinvented slightly differently by the second and third composable.
  4. Same new-unit-plus-sibling-spec rule as src/utils//src/components/ today. src/composables/useScrollArrows.ts lands with src/composables/useScrollArrows.spec.ts in the same commit — this is also where the src/composables/ directory convention itself gets established.
PR 2 — once PR 1 is proven, extend deliberately to the rest

Not opportunistic, not "whoever next touches that region" — a committed second PR, once PR 1's shape has held up in review and in practice. Targets:

  • OfficeOverview.vue: the create-from-template dialog cluster (showCreateDialog, newFileName, pendingCreator, pendingTemplate, creating, createError, createInput)
  • OfficeOverview.vue: the view-mode toggle + persistence cluster (viewMode, toggleViewMode(), getOverviewGridView/setOverviewGridView)
  • OfficeOverview.vue: the active-creator/routing-sync cluster landing via #130 (activeCreator, routeCreatorId, the URL-sync watch)

Each of these is its own commit within PR 2, one concern per commit, same as always — but landing as one deliberate pass rather than trickling in over unrelated future feature work.

After both PRs: documenting the pattern

Once PR 2 lands and the model has held up across all four clusters — not before — #139 writes it into AGENTS.md as an established pattern. Kept as a separate issue rather than folded into this one, so this issue stays scoped to the code work.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with TemplateSection.vue and check @vueuse/core's useScroll before extracting the scroll-arrows cluster. Add src/composables/useScrollArrows.ts, its sibling spec, and the lifecycle-hook test helper in src/test-utils/. The first PR is done when this proof of concept is tested and the accessibility gap is addressed; only then should the separate PR extend the pattern to the listed OfficeOverview.vue clusters.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
frontend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.