Fix Comparison Plot "Image By Step" height

[julieg18]

PR

Screen.Recording.2023-08-09.at.6.22.08.PM.mov

Main

257299504-96067397-e567-4f23-be5a-369d156b39a4.mov

Part of #4422

[julieg18]

Anyone have a better idea on how to accomplish keeping the images the same height?

[mattseddon]

[Q] Are you able to reproduce the broken behaviour from #4372 on main with height: 380px; removed from .multiImageWrapper? I wanted to see if simply setting img.styles.height here and not using useState worked but I cannot recreate the original issue at all on main when the fixed height is removed (I logged a lot of images with multiple experiments). Could the original issue have been caused by the syncing of the slider/a race condition?

[julieg18]

Are you able to reproduce the broken behaviour from #4372 on main with height: 380px; removed from .multiImageWrapper?

Yes, I'm still able to reproduce. Here's a demo:

Screen.Recording.2023-08-10.at.9.32.16.AM.mov

Images need to be same height so the slider doesn't "jump".

I wanted to see if simply setting img.styles.height here and not using useState worked

Oh, that should work! I'll try it out.

[julieg18]

Tested setting img.styles.height on the images directly instead of using useState! Realized that we'd have to set img state directly on to all images in the row and we'd have to add an detection to see when a new image is added so we could update the height for that image which makes things more complicated.

But we could apply --img-height directly on to the multi image row wrapper instead of each individual cell. That way we can avoid using useState and not need to watch for image addition. I'll update the code :)

[julieg18]

Tried adding a test but it would fail:

  it('should update the multi image height when changing the width of plots', async () => {
    const store = renderAppWithOptionalData({
      comparison: comparisonTableFixture
    })
    setWrapperSize(store)

    const multiImage = screen.getAllByTestId('multi-image-cell')[0]

    const originalHeight =
      getComputedStyle(multiImage).getPropertyValue('--img-height')

    await waitFor(
      () => {
        const imgEl: HTMLImageElement = within(multiImage).getByRole('img')
        expect(Boolean(imgEl.complete)).toStrictEqual(true) // fails
      },
      { timeout: 1000 }
    )

    sendSetDataMessage({
      comparison: {
        ...comparisonTableFixture,
        width: 3
      }
    })

    await waitFor(
      () => {
        expect(
          getComputedStyle(multiImage).getPropertyValue('--img-height')
        ).not.toBe(originalHeight)
      },
      { timeout: 1000 }
    )
  })

I think it's due to how the virtual dom deals with images 🤔. Any idea on how to test for this?

[mattseddon]

Have to setup and take a snapshot in Chromatic

[julieg18]

Adjusted our storybook images to include one of different size and added an extra snapshot!

[mattseddon]

This is fine, but let's double-check that we actually need it instead of simply removing the fixed height.

[mattseddon]

[Q] Are you able to reproduce the broken behaviour from #4372 on main with height: 380px; removed from .multiImageWrapper? I wanted to see if simply setting img.styles.height here and not using useState worked but I cannot recreate the original issue at all on main when the fixed height is removed (I logged a lot of images with multiple experiments). Could the original issue have been caused by the syncing of the slider/a race condition?

[codeclimate]

Code Climate has analyzed commit cbf50f9 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 61.1% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.2% (0.0% change).

View more on Code Climate.