Repository navigation
Conversation
Author
|
Closing: this work is now on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Adds a reusable
app-panel-layout/app-panelpair (collapsible, resizable, full-screen panels that stack into tabs on a narrow screen) and moves the staff task inbox and the student project dashboard onto it, replacing two separate hand-rolledcdkDragresize implementations.Stacked PR
This branch is based on
theme/status-and-globals(open as #192).git merge-base theme/status-and-globals ui/panel-layoutis1ec29efb0, the tip of that branch. The diff shown here is only this slice's incremental diff on top of that base, not the whole branch, and it must not be merged before it.The stack is a DAG with a merge point, not a straight chain. Checked with
git merge-base --is-ancestor:ui/panel-layoutandui/a11y-auditare siblings. Neither is an ancestor of the other, and their merge base iscc4a834e6, the first commit in this slice.ui/notification-linksand ofui/inbox-extension-card. They come back together in7972ea62a(Merge branch 'ui/panel-layout' into ui/inbox-extension-card) and3c19fb377(Merge branch 'ui/a11y-audit' into ui/inbox-extension-card), and both merge commits are ancestors ofui/notification-links.ui/notification-linksis an ancestor ofui/inbox-extension-card.So the order is
theme/status-and-globals→cc4a834e6→ui/panel-layoutandui/a11y-auditin either order → the two merges onui/inbox-extension-card→ui/notification-links→ theui/inbox-extension-cardtip.What changed and why
A shared panel primitive.
PanelLayoutComponentlays outapp-panelchildren side by side in an even 12px frame. Each panel can collapse to a 44px icon rail, resize by dragging its edge or with the arrow keys, or go full screen. At or below thestackBelowwidth, 899.98px by default, the layout switches to one panel at a time behind a row of tabs instead of shrinking everything, so nothing scrolls sideways on a phone-width screen.PanelComponenttalks to its host layout through aPANEL_LAYOUTinjection token kept in its own file, so the two components don't need to import each other.ontrack.panels.<page>.<panel>inlocalStorage) byPanelStateService, and width is remembered for the panels that can resize, meaning aresizeEdgeand noflex. Every read and write is wrapped in try/catch: a private window, blocked site data, or a full quota just means the panel falls back to its default instead of breaking.minWidth/maxWidth) with an arrow-key equivalent (shift for a bigger step) on the same resize handle, which the old CDK-drag implementation did not have. The old handles werearia-hiddendivs with no tabindex and no keydown.inertwhile one is full screen, so Tab cannot reach what it covers. That is the same scope the dashboard's old[attr.inert]="commentsFullscreen"bindings had, and it does not cover the page outside the layout.fullscreenableon either page, and only while there is a task behind it: the inbox binds[fullscreenable]="!!taskData?.selectedTask"and the dashboard's comments panel only renders inside@if (selectedTaskDefinition). The list and the middle "current item" panel are never fullscreenable. The middle panel is also not collapsible or resizable, it just takes the space the other two leave (flex).Migrated the task inbox and project dashboard onto it. Both pages had their own
cdkDragresizer, their own width clamping and their ownsplit-pane-resizingbody class toggled by hand. The inbox also threw its drag events through asubs$/auditTime(30)pipeline. The dashboard'ssubs$was justof(true)and it clamped on every move event. All of that is now the shared component's job in both places.The dashboard's floating "show comments" button for a narrow desktop window is gone along with all its CSS. The inbox's copy is gone from the desktop branch only. Its
.comments-floating-togglerules stay ininbox.component.scssbecause the phone branch still uses that button.Extension request card restyled in
cc4a834e6. It is now a header/reason/footer card with status chips (Granted, Denied, Awaiting decision) instead of the old stacked-text layout, and the Grant/Deny buttons disable while a request is in flight (a newbusyflag on the component) so a slow response cannot be double-submitted. Note thatcc4a834e6is the merge base withui/a11y-audit, so this change is not unique to this branch and will show up again in that sibling's diff from the same base. It only needs reviewing once.What the migration left alone, and what moved anyway
@elseon!isMobileView) is unchanged in this diff. Only the desktop branch was migrated.mobilePanetab bar (Overview, Tasks, Details, Feedback) rather than the shared layout's tab stacking, and the panes behind it still render the same components. The markup did move though: the tab bar and the pane row now sit inside the@if (isPhoneLayout)branch, theisPhoneLayout &&guards came off theproject-task-list-panel--phone-hiddenconditions, the inline overview skeleton moved into anOverviewSkeletontemplate, and the old.project-task-resizerelement and its--phone-hiddenrule are gone.project-dashboard.mobile.spec.tsfollows those selectors in this diff.extension-comment.component.tsbefore or after this change, so the newbusy-disables-the-buttons behaviour has no automated coverage in this slice.Testing
Net across the spec files touched: 9
it(...)cases added, 2 removed. No test run is reported here.panel-layout.component.spec.ts, 7 cases, exercisingPanelLayoutComponentandPanelComponenttogether through a 3-panel host template that mirrors real usage (list / flexible work area / fullscreenable comments). Covers: collapse to rail and back, the headerless edge-toggle collapse control, remembering collapsed state and width per page+panel key, falling back to defaults when storage throws, ignoring a stored value of the wrong shape, going full screen (the other panel inert, Esc to exit), and stacking into tabs with collapse ignored while stacked.project-dashboard.component.spec.ts, 2 cases renamed and simplified for the newfullscreenPanel: string | nullfield replacingcommentsFullscreen/commentsNarrow$. It stays at 12 cases. The toggle-on-and-off assertions those two cases carried are now the layout's behaviour and live in the panel-layout spec instead.project-dashboard.mobile.spec.tshad its selectors and breakpoint mocks updated for the new markup. It stays at 13 cases, none added or removed.inbox.component.ts's wiring intoapp-panel-layout(its existing spec overrides the template with an empty string and has oneshould createcase), and the extension-comment restyle noted above.Review guidance
1101 of the 1521 added lines are the new
panel-layoutdirectory, which is 7 new files with nothing removed. It carries all of the new interactive logic and has its own spec.panel.component.ts(drag/keyboard resize, collapse, full screen, inert) andpanel-layout.component.ts(breakpoint stacking, Escape handling) are where a bug would actually live.panel.component.html/.scssandpanel-layout.component.html/.scssare mostly template and styling for that logic.The riskier review, behaviourally, is the migration itself:
inbox.component.ts/.htmlandproject-dashboard.component.ts/.html, since that is where an existing page's resize, collapse and full-screen behaviour has to keep working, and where widths that used to live in component state now come out oflocalStorageunder a new key. The dashboard's phone branch is worth a second look for the same reason, because its markup moved even though the panes did not change.doubtfire-angular.module.ts(the two new components added toimports) and the extension-comment restyle are mechanical or visual and lower risk.Merge order
The base branch is
theme/status-and-globals, open as #192. Nothing here merges before it.After #192, this PR and the accessibility-audit PR are independent of each other and can land in either order. Both must land before the notification-links PR, which already contains the merge commits for both.
One overlap to know about: commit
cc4a834e6(the extension request card restyle and the first full-screen comments toggle) is the fork point this branch shares with the accessibility-audit branch, and it is not ontheme/status-and-globals. It is 6 files, +247/-68. Whichever of the two PRs merges first carries it, and the second will show it already applied. It only needs reviewing once.