search

flatironinstitute / mcmc-monitor

Monitor MCMC runs in the browser
Other
35 stars 0 forks source link

Test tab widget #93

Open jsoules opened 1 year ago

jsoules commented 1 year ago

Adds some tests for the public behaviors of the TabWidget and TabWidgetTabBar.

I did some reorganizing of the TabWidget class to remove some logic that supported a commented-out feature, and to implement memoization for the displayed pane. I'm not sure if this would have performance implications due to eager rendering of some panes--so we might want to think about that a bit.

I also just set the currentTabIndex to be non-nullable with a default of 0, since the prior implementation appears to set it to 0 through a hook if it does happen to be undefined.

magland commented 1 year ago

Could you please deploy this to dev folder? I want to check something about the tab visibility functionality.

magland commented 1 year ago

There's a problem (even with existing version before this PR) where if you manipulate the view on one tab (say collapse the display for some variables) then navigate to another tab, and then come back... everything is reset. Would be better if it could remember the current state of that tab. I think that's what the hasBeenVisible variable was trying to do. I guess we should open a separate issue for this one.

But this PR overall looks good to me.

jsoules commented 1 year ago

Just saw this--deployed to the dev site, but it may take a couple minutes for it to show up.

There's a problem (even with existing version before this PR) where if you manipulate the view on one tab ... then navigate to another tab, and then come back... everything is reset.

Oh, yes. I think that's the problem that issue #21 refers to (though Brian mentioned the checkbox state rather than variable collapse state).

I suspect (though I am not certain, I haven't looked into it with any rigor) that the issue here may be related to the way the right-panel widgets are set up in RunPage.tsx--it looks like they get recreated from scratch every time that renders, so their internal state is getting clobbered too. One option might be to memoize those, but I think it might also require bumping their internal state up a level?

But this PR overall looks good to me.

Great! But just in case, I'll give you a chance to look at the dev site first & wait to hear back before I merge.