Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track viewed tabs in TabWidget component #144

Merged
merged 5 commits into from
Jul 24, 2024
Merged

Conversation

magland
Copy link
Collaborator

@magland magland commented Jul 24, 2024

This was discussed in #142

With this PR: In tab widget, tab content is not removed from DOM when switching between tabs, but also the tab content is not included in the DOM until the tab was viewed at least once. This involves keeping a state tabsThatHaveBeenViewed.

To verify this is working:

  • Run sampling and click on ANALYSIS (PY) tab
  • Resize the editor
  • Switch to the OUPUT tab and then back to the ANALYSIS tab
  • Note that the resize has persisted

Then for a second test:

  • Run sampling and wait for a few seconds
  • In the OUTPUT tab, click HISTOGRAMS tab
  • Note that plotly is being loaded, demonstrating that it hadn't been loaded previously.

@magland magland requested a review from WardBrian July 24, 2024 12:20
Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts on the implementation of this. I agree it’s the right thing to do in general

@@ -15,6 +15,17 @@ const TabWidget: FunctionComponent<TabWidgetProps> = ({ labels, children }) => {
}

const [index, setIndex] = useState(0);
const [tabsThatHaveBeenViewed, setTabsThatHaveBeenViewed] = useState<
number[]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on using a boolean[] of the same length as labels, rather than a number[]?

gui/src/app/components/TabWidget.tsx Outdated Show resolved Hide resolved
gui/src/app/components/TabWidget.tsx Outdated Show resolved Hide resolved
gui/src/app/components/TabWidget.tsx Outdated Show resolved Hide resolved
@magland
Copy link
Collaborator Author

magland commented Jul 24, 2024

@WardBrian I made all your suggested changes/simplifications, except for your changing the state type to boolean[]. I think it's cleaner as number[] because

  • More natural to initialize as []
  • May want to accommodate dynamic changes to number of tabs in future (at that point we'd want to use tab IDs rather than indexes)
  • State updates should create immutable objects. It's more natural to update a list compared with updating an entry in a boolean[] in an immutable way.

@jsoules
Copy link
Collaborator

jsoules commented Jul 24, 2024

@WardBrian I made all your suggested changes/simplifications, except for your changing the state type to boolean[]. I think it's cleaner as number[] because

* More natural to initialize as []

* May want to accommodate dynamic changes to number of tabs in future (at that point we'd want to use tab IDs rather than indexes)

* State updates should create immutable objects. It's more natural to update a list compared with updating an entry in a boolean[] in an immutable way.

An additional possibility would be to just use a string[] whose values are the labels. If we are planning for the situation where tabs might be dynamic, we might also want to support reordering, which would be difficult with an index-based or order-based numeric ID.

But I don't really feel too strongly about this--I think it'll be easy enough to adapt the solution if we ever decide to support those features.

@WardBrian
Copy link
Collaborator

Yeah, I'm happy with the current solution and the options we have for crossing-bridges-should-we-come-to-them

gui/src/app/components/TabWidget.tsx Outdated Show resolved Hide resolved
@WardBrian
Copy link
Collaborator

Also note, re #142, it appears that while the tab stays mounted, as you observe with the splitters maintaining their states, the pyodide worker does seem to terminate still. I think there is a separate set of changes deeper in the call stack needed to fix that, but I'm also planning on proposing some refactors to those component's organization that I think would fix it anyway.

So besides the one comment above I think this can be merged as-is

@magland magland merged commit ad19ff6 into main Jul 24, 2024
2 checks passed
@jsoules jsoules deleted the tab-widget-rendering branch July 24, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants