Page MenuHomePhabricator

[dagit] Consolidate pieces into "Instance details"
ClosedPublic

Authored by dish on Jan 5 2021, 9:23 PM.

Details

Summary

Proposed changes to the "Instance details" page and left navigation, following Scheduled Runs chat earlier.

  • Remove "Daemons" page in favor of tabs on "Instance details" page, as a location to render instance-level information about schedules and sensors.
  • Move "Instance details" link down to be a top-level link rather than a mini-link next to the version number.
  • Move and resize left nav items following the above change.
Test Plan

Verify changes described above.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 5 2021, 9:28 PM
Harbormaster failed remote builds in B23664: Diff 28777!
dish requested review of this revision.Jan 5 2021, 9:47 PM

This looks great to me! I'm a big fan of this change because I think it'll more clearly communicate the repo vs. instance distinction between the two UIs that show sensors and schedules ๐Ÿ‘

js_modules/dagit/src/instance/InstanceSchedules.tsx
43

This isn't super relevant to this diff, but I've been working with another internal library recently that writes out spacers explicitly. So far I've found it's a little easier to visualize what the UI looks like than with nested groups where the spacing declaration is further away from where the spacing actually appears, might be an interesting exploration one day:

(Eg:

<Group direction="column">
    <SchedulerTimezoneNote schedulerOrError={scheduler} />
    <Spacer size={12} />
    <SchedulerInfo schedulerOrError={scheduler} daemonHealth={instance.daemonHealth} />
    <Spacer size={32} />
    {withSchedules.map((repository) => (
    <>
        <Subheading>{`${repository.name}@${repository.location.name}`}</Subheading>
        <Spacer size={12} />
        <SchedulesTable
            repoAddress={{name: repository.name, location: repository.location.name}}
            schedules={repository.schedules}
        />
    </>
    ))}
</Group>
This revision is now accepted and ready to land.Jan 6 2021, 4:38 PM

Agreed, I like this a lot better

<Subheading>{`${repository.name}@${repository.location.name}`}</Subheading>
<Spacer size={12} />
<SchedulesTable
  repoAddress={{name: repository.name, location: repository.location.name}}
  schedules={repository.schedules}
/>

My thought on this is that we should probably have a Section component that encompasses and standardizes the subheading+spacing+content, which should help us deduplicate some of the nesting and repetition.

Add 'Instance' header and rename page, following discussion.

dish retitled this revision from [dagit] RFC: Consolidate pieces into "Instance details" to [dagit] Consolidate pieces into "Instance details".Jan 7 2021, 8:27 PM

Test, use CSS for uppercase headers