[dagit] Sectioned left nav #8017
Conversation
DAGIT-34 Group left nav by repository
Organize all the jobs in the left nav by repository.
Mock: |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
hellendag
force-pushed
the
dish/dagit-34-group-left-nav-by-repository
branch
from
dcd1077
to
69824cb
Compare
|
|
There was a problem hiding this comment.
This looks really great - I left a couple comments inline but nothing blocking, I'm excited to play around with this behind the flag and see if we can ditch the old version.
Curious if @braunjj has thoughts about whether we keep the "4 / 7 repos shown" behavior now that things can be collapsed? Maybe it's more useful to have both layers if you're grouping on another axis?
| const EXPANDED_REPO_KEYS = 'dagit.expanded-repo-keys'; | ||
|
|
||
| export const SectionedLeftNav = () => { | ||
| const {loading, visibleRepos} = React.useContext(WorkspaceContext); |
There was a problem hiding this comment.
I think it may be beyond the scope of this PR, but long term I'm not sure we want to have both hidden and collapsed, it may be ok to show all repos as collapsed all the time? The only scenario I can think of where you'd still want to HIDE a few is if you had a ton of them. (15+?)
There was a problem hiding this comment.
I think that many-repo case might be a genuine scenario that we need to cover. Either way, it's easy enough to change. :)
| const {basePath} = React.useContext(AppContext); | ||
|
|
||
| const [expandedKeys, setExpandedKeys] = useStateWithStorage<string[]>( | ||
| basePath + ':' + EXPANDED_REPO_KEYS, |
There was a problem hiding this comment.
It'd be interesting to explore baking this prefixing into useStateWithStorage. I can't think of any cases where this would be detrimental and I don't think I've been doing it...
| const reposWithoutJobs = []; | ||
| for (const repo of alphaSorted) { | ||
| const jobs = repo.repository.pipelines; | ||
| if (jobs.length > 1 && jobs.some((job) => !isAssetGroup(job.name))) { |
There was a problem hiding this comment.
Hmm I think this should be jobs.length > 0, because it'd be possible to have a repo with no assets that doesn't expose an asset group job and only one standard job? i don't /think/ asset group jobs are generated for repos that have no assets.
| overflow-x: hidden; | ||
| `; | ||
|
|
||
| const SectionHeader = styled.button<{$open: boolean; $showRepoLocation: boolean}>` |
There was a problem hiding this comment.
Props for using a nice semantic tag for this!
| <BaseTag | ||
| fillColor={Colors.Gray10} | ||
| textColor={Colors.Dark} | ||
| label={new Intl.NumberFormat().format(jobItems.length)} |
There was a problem hiding this comment.
I think we do this elsewhere with Number(1000).toLocaleString() , maybe they're short-hands for each other? (Actually need to read up on this version...)
hellendag
force-pushed
the
dish/dagit-34-group-left-nav-by-repository
branch
from
69824cb
to
d03fde3
Compare
Summary & Motivation
Split the left nav into expandable repository sections, with the list of the repo's jobs rendered below it.
This is behind a feature flag, settable at User Settings.
Will follow with a Jest test once we're settled on the layout and interactions.
How I Tested These Changes
View Dagit and Storybook example. Verify correct rendering, disabled state, expandability, collapsibility, job count, scrollability.