- User Since
- May 9 2019, 3:45 PM (95 w, 20 h)
This seems like a nice presentation - I think we've been struggling for a while to find a consistent home for "mode" that doesn't make it look like a tag. I like having the <PipelineAndMode> component to wrap up the UI - I wonder if we could push the href generation down into that component in any way? I'm not sure how easy it'd be to retrieve repoAddress within the component...
Oh this is super cool - nice that we don't need moment for this sort of thing these days.
Mon, Mar 1
This looks great to me! Also agree with getting rid of the search bars in each section 👍
Tue, Feb 23
This looks awesome! Thanks for writing tests too since I imagine this will get pulled into the other places we have a tokenizing text field.
Mon, Feb 22
This looks good to me, I don't think the capitalized strings in the logs are too scary...
This looks great! Nice we already had useRepositoryLocationReload, I forgot it was a hook!
This looks great to me! It does seem it'd be nice to have the fuse implementation pulled out into a helper hook because it gets a little technical, but there may not be anywhere else that we could re-use it so it's probably not worth factoring apart into two pieces.
This looks good to me!
Mon, Feb 15
I'll let other folks weigh in but the JS for this looks great. I think that you're right - the dot should either display detailed info when you hover over it (as opposed to Warnings found) or they should be in a consolidated spot on the instance tab (or maybe each tab of the instance UI gets it's own yellow dot so you can click into the right tab? Probably overkill...)
This looks great! I like the new shortcut annotation styling too, matches the new sidebar a bit better 👍
Thu, Feb 11
Oh this is great, I really like the slow pulsing of the queued runs. I feel like it /could/ be a little distracting if different runs on the page pulse out of sync, but I imagine there aren't many places where we'd run in to that. Let's ship it!
Oh wow this is super noticeable. I didn't think to try this, looks much nicer!
Tue, Feb 9
This looks great! I like this direction and the consistency of the header + subtitle approach a lot. Just have a couple drive-by comments below, but I think we should merge this ASAP.
Fri, Feb 5
Yep I think this will do it 👍 I think this must have become a problem when we started emitting logs during the "queued" phase and putting a big gap between firstLogAt and startedPipelineAt.
This looks like a great refactor to me! Big fan of making these consistent.
Feb 2 2021
Ah I like this styling above the input, good call!
Ahh this is exciting! Look forward to trying it in the app - the code looks great to me 👍
Feb 1 2021
I think we talked about this Thursday - I'm a big fan of incrementally introducing the repository concept and having this flat list of pipelines. The components look good to me!
Nice! I think that longitudinal_pipeline also has a partitioned costs_dashboard asset and I've been using that one to test the asset materialization views but it'll be nice to have one that emits metadata entries.
Looks good to me! I'm surprised timestampToString is used outside the time folder since it's not user-settings-aware, but we can audit that later!
Looks good to me!
This looks great and I like the banner styling a lot. I guess there's always a React render after the refresh function is run, so invalidateConfigsForRepo doesn't need to trigger a render after it modifies local storage? It might be good to add a small note to that function to note that the caller should trigger a page refresh in some way, because interacting with the config editor after calling invalidateConfigsForRepo would overwrite the local storage state back to match the config editor's state (I think?) A cheap alternative might be to fire a custom event on the document that components could listen to and handle a repo reload, and then set the dirty bit from the ExecutionSessionContainer, but I think this is clear and is less code!
Looks like a safe bet to me! There are so many spinners on here.
This looks good to me! Big fan of standardizing these, even if it makes things less pixel-perfect in a button or two. 🙌
Hmm this was definitely intentional but I don't quite remember the context around it. I went back and found the original diff where that was added (https://github.com/dagster-io/dagster/commit/02b1d07b6b00407c9533d4d718efc6195620961a#diff-0788ce9e6243e61675f971ee6022361b57a9d1a0b32fcff11036acf8583e22fb) but there's not much of an explanation and it's almost two years old. I'd say we merge this and give it a spin, and if there's interest in opening things in separate windows we could potentially allow cmd-clicking on the buttons, etc to send openInNewWindow: true.
Jan 31 2021
Rebase and address diff feedback
Jan 26 2021
Jan 25 2021
I think this is a positive change - removing the alternating background seems fine and makes the rows appear less clickable (which I think is a *good* thing because our rows are largely not clickable.)
Oh good catch—seems like a major oversight that this wasn't awaiting the mutation!
This looks great to me!
Jan 23 2021
- Instead of flipping, scroll the asset materialization grid to the right by default
- Fix React warnings in the useViewport component, some useCallback's missing
Jan 19 2021
Ahh this is great! +1 on this being a blocker on switching to functional components without weird Typescript hacks. I don't think I've run into the circular dependency issue, but I imagine that's a cryptic rabbit hole we should avoid!
Jan 15 2021
Jan 8 2021
Wow this is a cool idea, code looks good to me! I like the highlighted styling too, nice UI bonus 🙌
Jan 6 2021
This is great! I think this is a really nice use of useReducer and I'm surprised it ended up being so concise, when you described this yesterday I imagined it being about 50% more code than this :-p Will be a nice feature to have!
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 👍
Update asset header to use Group
Jan 4 2021
This looks great to me!
Dec 23 2020
This looks great to me! Cool use of tabular-nums in here too, I forgot that was a thing!
Will defer to @sandyryza for Python review but this looks good! Excited to get this in!
Dec 21 2020
Sounds good! Yeah I agree doing this on hosted dagit would be bad. There are a few improvements in this diff separate from the actual HTML / JSON / inlining feature and I'll pull those into a separate tiny diff here in a bit.