Page MenuHomePhabricator

Add custom behavior to the CellTruncationProvider
ClosedPublic

Authored by prha on Wed, Aug 14, 10:03 PM.

Details

Reviewers
bengotow
Group Reviewers
Restricted Project
Commits
R1:d79b27377be1: Add custom behavior to the CellTruncationProvider
Summary

The CellTruncationProvider could only show raw text in a dialog
because it only had access to generated DOM, instead of react elements. This
diff adds an event handler to the CellTruncationProvider to allow for customized
expansion behavior (when a user clicks on 'View Full Message').

This diff pushes the CellTruncationProvider down into a new <LogsRow />
component, which has branching behavior based on the event node type. The
original structured content rendering logic was extracted into a separate
component (LogsRowStructuredContent), so we can render the exact same content
in the truncated table view as well as the unabridged modal view.

CellTruncationProvider becomes a little more generic (no longer tied to the
structure of the rendered DOM) at the expense of forcing each event type to
consider the truncated view flow.

Issue: https://github.com/dagster-io/dagster/issues/1660
Related Issue: https://github.com/dagster-io/dagster/issues/1623

Test Plan

Rendered Unstructured, Failure, Materialization events, resized window,
triggered the 'View Full Message' link.

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

prha created this revision.Wed, Aug 14, 10:03 PM
prha edited the summary of this revision. (Show Details)Wed, Aug 14, 10:08 PM
prha added reviewers: Restricted Project, bengotow.

this looks good to me but ill give @bengotow time to look

js_modules/dagit/src/runs/CellTruncationProvider.tsx
74–77

want to fix this (use ref) while you're here?

prha added inline comments.Thu, Aug 15, 4:01 PM
js_modules/dagit/src/runs/CellTruncationProvider.tsx
74–77

Yeah, I spent maybe 45 min yesterday trying to get ref working here, transferring it to the child prop, but couldn't figure it out.

I should at least leave the original github issue comment here.

prha updated this revision to Diff 3726.Thu, Aug 15, 4:30 PM
  • Use refs instead of ReactDOM.findDOMNode
prha added a comment.Thu, Aug 15, 4:32 PM

ended up just wrapping the content with a wrapper div... I don't think there's another way to do it

bengotow accepted this revision.Thu, Aug 15, 6:47 PM

This looks great! This approach definitely seems more flexible.

This revision is now accepted and ready to land.Thu, Aug 15, 6:47 PM
This revision was automatically updated to reflect the committed changes.