coopermaruyama / tableau-react

Tableau React component integrated with Tableau JS API.
MIT License
77 stars 60 forks source link

fix: remove sheet nesting #19

Closed ivanmcgregor closed 4 years ago

ivanmcgregor commented 4 years ago

I tried with our own graph on tableau public, as well as the one from the tutorial ( https://public.tableau.com/views/WorldIndicators/GDPpercapita ). In both cases the issue is, that getActiveSheet() is already returning the sheet, so there is no method getWorksheets(). This is only failing the function, but whenever we try to change the filter, it is crashing the entire component.

@coopermaruyama do I also have to run the build job locally and commit the changed dist folder or will this run automatically?

coopermaruyama commented 4 years ago

Hi there, thanks for the pull request and contribution.

I made a demo here using the graph you posted and it seems to work unless I'm missing something?

See demo: https://codesandbox.io/s/tableau-react-ylo5u

If we are going to assume that getActiveSheet() may return the first sheet in some cases, then we should also make it backwards compatible so it doesn't break for other graphs.

ivanmcgregor commented 4 years ago

Hi @coopermaruyama, If you have a look at the console output of your example you will already see image So, in essence, the assigning of a sheet for later usage is already failing. If you were to build an example where you pass in filters and change them, say with a timeout of 2 seconds, the code will then try to access the not properly assigned sheet and die.

coopermaruyama commented 4 years ago

@ivanmcgregor okay I see. I haven't used tableau in years, but combing through the docs and doing some testing on other sheets, it looks like it works correctly if the active sheet is a dashboard.

After years of this library being published I'm very surprised this hasn't come up, my only guess is that it's common for users to create dashboards and expect the filter to be applied to the first one.

I'm going to accept this change but make a slight modification that will keep existing behavior for current users (if there are nested sheets, choose the nested one).