[PER-10415] Edtf date record viewer - #1065
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PER-10643-edtf-validation #1065 +/- ##
=============================================================
- Coverage 52.64% 52.63% -0.02%
=============================================================
Files 354 354
Lines 12199 12236 +37
Branches 2220 2227 +7
=============================================================
+ Hits 6422 6440 +18
- Misses 5549 5566 +17
- Partials 228 230 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fe7dd07 to
57c3de4
Compare
67e2e7f to
dd1b3c1
Compare
|
+1 to @slifty 's comments and I'll remove myself as a reviewer for now. |
57c3de4 to
67edbff
Compare
Very good points, I've addressed all of them. |
dd1b3c1 to
df091d0
Compare
5381112 to
6458a27
Compare
|
@aasandei-vsp I'm back from vacation -- I imagine we'll catch up on standup shortly about the status of this PR / if you're looking for another review! |
|
@slifty, I've been working with @aasandei-vsp on #1056, which I've just approved. This one probably needs conflicts updated because of our work over there, and then yes I expect it is ready for another round of review. Correct me if I'm wrong about any of that, @aasandei-vsp ! |
6458a27 to
80e59ec
Compare
Gate the date control on the edtf-date flag: show the EDTF dropdown/modal when it's on, and the legacy inline date field when it's off. Port the sidebar's date handling to the file viewer: - Cache displayTimeObject in a field, recomputed per record in initRecord, instead of a getter that allocated on every change-detection cycle. - Re-sync after every save via a shared saveDisplayTime helper (used by both the inline picker and the modal), including on failure. - Track the modal's closed subscription so it unsubscribes on destroy. Issue: PER-10415
The global error banner sat at a lower z-index than full-screen views (e.g. the file viewer), so errors rendered but were hidden behind the overlay. Raise it above the full-screen layer and the date/time picker dropdowns that open within it. Issue: PER-10415
Mirror the sidebar behavior: persist null instead of '' when a date is
cleared, and render an explicitly-null displayTime as empty ("Click to
add date"). Add specs.
Issue: PER-10643
df091d0 to
f497b06
Compare
|
@cecilia-donnelly You are exactly right about the situation :D |
There was a problem hiding this comment.
Pull request overview
Adds a feature-flagged EDTF date editor to the full-screen file viewer, aligning the viewer’s Date UI with the newer EDTF-based sidebar/editor flow while preserving the legacy inline Date editor when the flag is off.
Changes:
- Introduces the EDTF date picker into the full-screen file viewer behind the
edtf-datefeature flag, hiding the legacy Date row when enabled. - Adds EDTF validation UX to the sidebar date picker (disable Save + show inline error message when the current model can’t be serialized to EDTF).
- Refactors shared EDTF error styling into a reusable SCSS mixin and raises the message banner z-index to ensure errors remain visible above overlays/dropdowns.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/styles/_mixins.scss | Adds a shared edtf-error-message mixin for consistent EDTF validation error styling. |
| src/app/shared/components/message/message.component.scss | Raises banner z-index so error messages aren’t hidden behind full-screen viewer/dropdowns. |
| src/app/file-browser/components/sidebar-date-picker/sidebar-date-picker.component.ts | Adds computed EDTF validation state, blocks Save when invalid, and centralizes DateTimeModel construction. |
| src/app/file-browser/components/sidebar-date-picker/sidebar-date-picker.component.spec.ts | Adds unit tests covering invalid/valid EDTF validation behavior and Save blocking. |
| src/app/file-browser/components/sidebar-date-picker/sidebar-date-picker.component.scss | Styles inline EDTF validation error and disabled Save button state. |
| src/app/file-browser/components/sidebar-date-picker/sidebar-date-picker.component.html | Renders validation error message and disables Save button when invalid. |
| src/app/file-browser/components/file-viewer/file-viewer.component.ts | Adds feature-flagged EDTF viewer integration, parsing/serialization, and modal “More options” handling. |
| src/app/file-browser/components/file-viewer/file-viewer.component.spec.ts | Adds tests for flag gating and EDTF parsing/saving/resync behavior. |
| src/app/file-browser/components/file-viewer/file-viewer.component.html | Conditionally renders EDTF picker vs. legacy Date row based on feature flag. |
| src/app/file-browser/components/edit-date-time-modal/edit-date-time-modal.component.scss | Switches modal EDTF error styling to the shared mixin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
slifty
left a comment
slifty
left a comment
There was a problem hiding this comment.
Copilot made a var name suggestion I agree with -- approving so you don't get blocked on needing another review after that though, looks good!
- Give the inline date picker the same live EDTF validation as the modal: save is disabled with an inline error while the input is invalid, sharing the modal's error styling via a new mixin. - Skip the date recompute in the file viewer when the edtf-date flag is off, so no EDTF errors can surface while the picker is hidden. - Await the property save in the file viewer so the picker re-syncs to the reverted value when a save fails on the server. Issue: PER-10415
f497b06 to
842cc6d
Compare
@slifty That is indeed much better, fixed! Thank you! |
Manual test cases — EDTF date in the full-screen record view
Setup: log in with the
edtf-datefeature flag on, upload a file, click it to open the full-screen record view. The metadata panel now shows a Date picker above the metadata table (the oldDaterow inside the table is gone).Warning
This PR also changes the inline picker's commit behavior everywhere it appears (file-list sidebar and the full-screen view): it no longer lets you attempt an invalid save and surface the error as a toast. It now shows a general error message inside the picker and disables Save, same gating as the modal. The "run in both places" tests from the PER-10643 test plan should be re-run with the new expectation below.
Note
Field-specific messages (invalid characters, out-of-range month/day, day-for-month) still appear inline below the offending input. What's new is the picker-level line above the footer: the generic
The date entered is not valid. Please check the values and try again.for anything with an inline message, andA complete date is required when time is provided.keeps its specific text (it has no inline field to attach to).Feature flag & placement
Flag on
Daterow inside the metadata table.Flag off
edtf-dateflag off and open a record full-screen.Daterow inside the metadata table with the legacy inline edit, no EDTF picker.General error & Save gating in the inline picker ⭐
(run in both the file-list sidebar picker and the full-screen view picker — same component, both surfaces must gate)
Invalid field value disables Save and shows the general error
13(or day32).The date entered is not valid. Please check the values and try again.The Save button is disabled (greyed out, not-allowed cursor).Impossible calendar day
2021, month02, day29.That day does not exist in the selected month and year.inline below the day input, the generic error above the footer, Save disabled.Time without a complete date
02:30 PM) with the date fields empty or partial.A complete date is required when time is provided.(no inline field message for this one). Save is disabled; clicking it does nothing and the dropdown stays open.Valid value
Saving from the full-screen view
Inline picker save
1985/05/20, Save.1985-05-20. No error toast.More options modal
1990), then click More options.Record without a date (displayDT fallback)
displayDTfallback), matching what the oldDaterow used to show.Clearing the date
nullis sent, not an empty string; no400from the backend). The picker shows no date.Failed save re-syncs the picker
2000-01-01, go offline (DevTools → Network → Offline) or block the update request, then save2010-06-15from the inline picker.2000-01-01instead of keeping the unsaved2010-06-15.Permissions
Viewer access
Unlisted share link
Public archive
Navigating between records in the viewer