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

Base key value on other filters to prevent unmount on selection #126

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

dmfalke
Copy link
Member

@dmfalke dmfalke commented Apr 27, 2023

This PR addresses the gd.emit is not a function error when selecting a subset in an eda histogram filter.

@dmfalke dmfalke requested a review from bobular April 27, 2023 04:18
@bobular
Copy link
Member

bobular commented Apr 27, 2023

I haven't tested yet, but quick question... I'm assuming the key was used here as a render refresh "hack"? It's not necessary as part of a grid or list, as far as I can tell. Ah, I see there's a comment to this effect!

// Note use of `key` used with HistogramPlotWithControls. This is a little hack to force
// the range to be reset if the filter is removed.

I'll test carefully!

@dmfalke
Copy link
Member Author

dmfalke commented Apr 27, 2023

Thanks! To be honest, that comment isn't super clear. If it's referring to the "viewport" range, I don't know why we would want it to reset. Also, changing the key prop wouldn't impact that.

I wonder if it was something related to an early implementation that is no longer relevant. It's possible we don't even need this key prop, but the current change minimizes the testing surface area.

Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

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

Nice catch!

Tested as many scenarios as I could. Draw filter, remove it with cancel button or chip [x] - change filter - etc. All worked fine.

@dmfalke dmfalke merged commit dbf5f64 into main Apr 27, 2023
@dmfalke dmfalke deleted the eda/fix-histogram-filter-unmount-on-selection branch April 27, 2023 23:49
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.

None yet

2 participants