Skip to content

feat: add Duration component#779

Merged
shariquerik merged 13 commits into
frappe:mainfrom
shariquerik:duration
Jun 14, 2026
Merged

feat: add Duration component#779
shariquerik merged 13 commits into
frappe:mainfrom
shariquerik:duration

Conversation

@shariquerik

@shariquerik shariquerik commented Jun 13, 2026

Copy link
Copy Markdown
Member
Screenshot 2026-06-13 at 12 21 34 PM

Coverage: 56.89% (+0.26% vs main)

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Minor nits — clean new Duration component; reuses the shared vocabulary well. One UX thing to confirm.

What's good (worth calling out, since this is the direction the library should move):

  • Reuses InputSize / InputVariant and extends InputLabelingProps instead of redeclaring (src/components/Duration/types.ts:1, :36) — exactly P5 + smell Figma UI Kit #3 done right.
  • v-model as number | null with update:modelValue, plus focus via defineExpose — no ad-hoc events.
  • format reuses the same name DatePicker already uses (src/components/DatePicker/types.ts:28), so no name drift on the formatting concept.
  • Ships tests, stories, Duration.md, and .api.md — full component shape.
  • Side change aligns TimePicker to the shared labeling contract (src/components/TimePicker/types.ts:24, label/description slots) — additive, non-breaking, good cleanup.

Nit:

  • src/components/Duration/Duration.vue:60 — an invalid commit refocuses the field, so a mouse user who clicks away gets pulled back until the text parses, is cleared, or they hit Esc. It's intentional and tested, but confirm the focus-trap is the behaviour you want vs. just showing the error and letting focus leave.

No blockers. format overloading preset + free-form template into one string prop is a little unusual, but it's documented, tested, and keeps the API to a single primitive — fine.

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — fresh pass, no specific angle asked.

Looks good — the invalid-commit focus trap from the last pass is fixed: focus now leaves on a bad commit and Duration.cy.ts covers it. API stays tight (InputSize/InputVariant, InputLabelingProps, v-model as number | null, format reusing DatePickers name). The TimePicker side-change is additive labeling/size forwarding — non-breaking.

One small nit:

  • Duration.vue:47 — the handleFocus comment says the if (isFocused.value) return guard preserves the in-progress (invalid) text when focus returns after an invalid commit. But commit() sets isFocused.value = false on the invalid path, so on re-focus the guard never fires and editValue is reset to the saved value. Behaviour is fine (you get a clean canonical edit), but the comment describes something that no longer happens — worth updating so the next reader isnt misled.

Not blocking.

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — fresh pass, no specific angle asked.

Looks good — the stale-comment nit from last pass is fixed: Duration.vue:43 now correctly says the guard is skipped after an invalid commit, so re-focus starts a fresh canonical edit. Matches the actual commit() flow.

API still tight — InputSize/InputVariant, InputLabelingProps, v-model as number | null, format reusing DatePicker's name, single string prop (not a config blob). Tests, stories, .md and .api.md all present. The TimePicker side-change is additive (size + labeling forwarding) — non-breaking.

Tiny thing, not blocking:

  • Duration.vue:62 — the invalid-commit comment says the rejected text "stays visible ... so the user can come back and fix it," but on re-focus handleFocus resets editValue to the canonical saved value, so the rejected text is discarded, not editable. Behaviour is fine; the comment slightly oversells it.

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — fresh pass, no specific angle asked.

Looks good — the last pass's stale-comment nit is fixed: Duration.vue:62 now correctly says the rejected text is discarded (not edited) on re-focus, matching the commit() flow. CI is green.

API stays tight — InputSize/InputVariant, InputLabelingProps, v-model as number | null, format reusing DatePicker's name as a single string prop. Tests, stories, .md and .api.md all present. TimePicker side-change is additive (size + labeling forwarding) — non-breaking.

One tiny thing, not blocking:

  • duration.ts:821parseColon doesn't bound minutes/seconds, so 1:75 parses to 135s and 5:99 to 399s rather than being rejected. Lenient parsing is a reasonable choice, but if you'd rather reject out-of-range parts, clamp/validate the m/s groups. Confirm it's intentional.

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — fresh pass, no specific angle asked.

Looks good — last pass's parseColon nit is resolved: duration.ts:824 now documents the out-of-range overflow (1:75→135s) as intentional stopwatch-style entry. CI green, tests cover presets, templates, parse, and the focus/commit flow.

API stays tight — InputSize/InputVariant, InputLabelingProps, v-model as number | null, format reusing DatePicker's name as a single string prop. Tests, stories, .md and .api.md all present. TimePicker side-change is additive (size + labeling forwarding) — non-breaking.

One tiny thing, not blocking:

  • types.ts:19DurationFormat = preset | (string & {}) accepts any string, so a typo'd preset (format="shrot") silently falls through to the template renderer and prints literally rather than erroring. Inherent to the string & {} trick and documented, but worth a mention for callers.

@shariquerik

Copy link
Copy Markdown
Member Author

/barista review

@barista-for-frappe

Copy link
Copy Markdown

Re-reviewing per @shariquerik — fresh pass, no specific angle asked.

Looks good — nothing left to flag. All prior nits are resolved and the component is in good shape.

  • API stays tight: InputSize/InputVariant, InputLabelingProps, v-model as number | null, format reusing DatePicker's name as a single string prop (not a config blob). No new boolean flags, no name drift.
  • Correctness checks out: focus/commit/escape flow is consistent (Duration.vue:43 guard, commit() clears isFocused on the invalid path), parseUnits rejects duplicate/trailing junk, parseColon overflow is documented as intentional.
  • Full component shape present: Duration.cy.ts (parse, format, focus/commit, escape, null-clear), stories/, Duration.md, .api.md.
  • TimePicker side-change is additive — it now consumes label/description/error/required/id from InputLabelingProps plus size, forwarded to its inner TextInput. Non-breaking (these weren't props before; only theoretical risk is a consumer who relied on label falling through to the root as an attribute, which the picker never supported).

No blockers. CI was still pending at review time — worth a glance before merge.

@shariquerik shariquerik merged commit c7973f3 into frappe:main Jun 14, 2026
6 checks passed
@shariquerik shariquerik deleted the duration branch June 14, 2026 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant