fix lingering issues from #57 following @timelyportfolio's approach

[joshhjacobson]

I was still having issues when an array of axes that included the rightmost axis was hidden. This gave the error TypeError: Cannot read property 'type' of undefined from this line in brushFor.js:

brushRangeMax = config.dimensions[axis].type 

I followed the same approach released in v2.2.5 to correct this.

[joshhjacobson]

@BigFatDog sorry to bother you, but is there any way we can get this PR merged in the next few days? I have a deadline coming up at the end of the month and it would be great to have this fixed by then.

[BigFatDog]

@joshhjacobson
Don't worry. I do it right now.

[joshhjacobson]

Hi there, just wanted to check in on this. Any chance you'd be able to merge this in the next day or two? If there are revisions that need to be made, please let me know. Thanks!

[BigFatDog]

I've been waiting for your response for the pending code review these days.
as I said in the end of review:

Please also don't worry this discussion would block your work. I'll merge ASAP once we have a conclusion.

If we reach a conclusion, I'll do this tonight.

[joshhjacobson]

My bad, I didn't realize you'd already completed a review. Where can I find that?

[BigFatDog]

Oh MY GOD! I didn't submit it...

Sorry, my bad.

[BigFatDog]

How about returning a dummy function when an axis is hidden:

 if (!config.dimensions.hasOwnProperty(axis)) {
    return () => {}
 }

// no more check
const brushRangeMax = ...
const yScale = ...

I'm not sure yet. Still investigating

[BigFatDog]

I see. @timelyportfolio's fix should be reserved.

In our case, my consideration is: when brushRangeMax is null, it may cause unexpected behavior to set brush extent:

const _brush = brushY(_selector).extent([[-15, 0], [15, brushRangeMax]]);

Therefore it returning a dummy function should be safer.

I've tested this with your original test case.

Please also don't worry this discussion would block your work. I'll merge ASAP once we have a conclusion.

[joshhjacobson]

Thanks for the feedback, I see what you mean. I'll implement this change right now.

[BigFatDog]

I'll be online to make sure this is merged tonight

[joshhjacobson]

Thanks so much!

[BigFatDog]

Sorry again for my mistake. I wish it didn't delay your work.

[joshhjacobson]

To clarify, did you want to take the following approach:

if (!config.dimensions.hasOwnProperty(axis)) {
  return () => {}
}

// no more check
const brushRangeMax = ...
const yScale = ...

which leaves the code something like this:

// handle hidden axes which will not be a property of dimensions
if (!config.dimensions.hasOwnProperty(axis)) {
 return () => {}
}

const brushRangeMax =
    config.dimensions[axis].type === 'string'
      ? config.dimensions[axis].yscale.range()[
          config.dimensions[axis].yscale.range().length - 1
        ]
      : config.dimensions[axis].yscale.range()[0];

const yscale = config.dimensions[axis].yscale;

i.e. change what @timelyportfolio had done for #57?

[BigFatDog]

keeping @timelyportfolio's fix unchanged.

Only adding this to the function:

const brushFor = (state, config, pc, events, brushGroup) => (
  axis,
  _selector
) => {
  // add this line
  if (!config.dimensions.hasOwnProperty(axis)) {
      return () => {}
  }
  // the rest code remain unchanged.
  const brushRangeMax = ....

[joshhjacobson]

Got it

[BigFatDog]

v2.2.7 has been published to deliver this PR.

Thank you for your time and efforts!
Good night. 🌔

[joshhjacobson]

Much appreciated!

[timelyportfolio]

@joshhjacobson @BigFatDog thanks so much. Sorry I missed that!

[joshhjacobson]

@timelyportfolio no problem! Wouldn't have been able to figure this out without your initial fix.

[BigFatDog]

Much appreciated!

You're welcome.

[BigFatDog]

@joshhjacobson @BigFatDog thanks so much. Sorry I missed that!

It's fine. Please don't worry about it.