Page MenuHomePhabricator

Fix hook issues, stop serializing modal contents to JSON

Authored by bengotow on Sep 3 2019, 6:26 PM.


Group Reviewers
Restricted Project
R1:3ce6be876c42: Fix hook issues, stop serializing modal contents to JSON

This diff fixes the new modals. Allowing the presenter to pass a React tree as the content of the alert makes the rendering fully flexible but didn't work because the internal implementation was serializing the alert to JSON.

This diff also fixes several regressions caused by the previous patch that moved a few things to the Hook syntax and exposed places where we need better test coverage. (It's surprising to me that the previous patch passed all the tests, actually.) I will write some additional tests in another diff.

Test Plan

Run tests

Diff Detail

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

Event Timeline

bengotow created this revision.Sep 3 2019, 6:26 PM
bengotow added inline comments.Sep 3 2019, 6:30 PM

This was a hard-fail bug because hooks can't be used in class components.

prha accepted this revision.Sep 3 2019, 7:01 PM
prha added a subscriber: prha.

This looks good to me....

re: CustomAlertProvider, I think we should use this sparingly, just to handle generic errors, as a catchall.

Most of the time, we'll want to just render a Dialog directly, which should allow for more UI control. The whole passing around body refs makes me nervous, because I have to think about the lifecycle more than I want to.

This revision is now accepted and ready to land.Sep 3 2019, 7:01 PM

Sounds good! The reason this exists vs. doing each dialog inline is just because the dialog doesn't animate open / closed unless it's container is in the DOM when it's open AND closed (set via the <Dialog isOpen={}> in this case). So if you're not careful, you can end up with scenarios where every single row of a table contains an invisible not-yet-opened dialog DOM tree (or you just lose the animation, which would be better).

prha added a comment.Sep 3 2019, 9:28 PM

yeah... maybe that's a complexity we can hide inside of a custom <DagsterDialog /> component, which would make sure we do the right thing with the animations by triggering the right events on the component lifecycle.

I think I remember tackling a similar problem on a different project and it being a little gnarly. But overall I prefer that API to the document Dialog instance.