feat: Display image in a graphics_card #1817

[dbranley]

The PR fulfills these requirements: (check all the apply)

• It's submitted to the main branch.
• When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
• When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
• If changes were made to ui folder, unit tests (make test) still pass.
• New/updated tests are included

Closes #1817

As requested in the ticket, I have made changes so that a background image can be displayed in ui.graphics_card. I added 3 new props to graphics.tsx - path, image, and path. I modeled these after similar props in image.tsx.

  /** 
   * The path or URL or data URL of the background image, 
   * e.g. `/foo.png` or `http://example.com/foo.png` or `data:image/png;base64,???`. 
   */
  path?: S  
  /** Background image data, base64-encoded. */
  image?: S
  /** 
   * The background image MIME subtype. One of `apng`, `bmp`, `gif`, `x-icon`, `jpeg`, `png`, `webp`. 
   * Required only if `image` is set. 
   */
  type?: S  

I then defined a new backgroundImageSrc const if one of these new props is defined.

          backgroundImageSrc = path 
            ? 'url('+path+')' 
            : image ? `url(data:image/${type};base64,${image})` 
            : undefined 

In the JSX code, I added a parent div to wrap the existing div. This parent div is where the background image is rendered, if it exists. A new div was needed since the existing div is defined with a 15px offset from each side of the containing block, which would prevent a background image on the existing div from covering the entire area of the card. So I felt the best way to cover the entire card was to add a new parent div for the background. Let me know if you think there is a better approach.

          <div style={{ backgroundImage: `${backgroundImageSrc ? backgroundImageSrc : undefined}`, 
                        backgroundSize: `${backgroundImageSrc ? 'cover' : undefined}`,
                      }}>

I then ran the make generate target to update the generated python and R code to reflect this new attribute in files, such as py/h2o_wave/h2o_wave/ui.py.

Locally, I modified the graphics_hilbert.py example to render a background image using a relative path to one of the out-of-box Wave images. Here is a screen shot of that:

I also created a new custom python example that renders a graphics_card without a background image and then 4 others that use the 3 variations of the path prop and another that uses the image and type props. Here is a screen shot of that:

I created a new unit test in graphics.test.tsx, where it passes dummy values and then checks that new parent div to make sure it has the background-image set properly. I remember before you said not to bother with visual unit tests, but I felt like something was needed here. If you disagree and think it should be removed, just let me know.

  it('Renders with background image', () => {
    const { queryByTestId } = render(<View {...graphicsBackgroundImageProps} />)
    expect(queryByTestId(name)).toBeInTheDocument()
    expect(queryByTestId(name)?.parentElement?.style.backgroundImage).toEqual('url(foobar.png)')
  }) 

I ran all the ui tests and confirmed they passed. Here is a screen shot for that:

I also ran the visual regression tool at tools/showcase. This was fine except for 1 mismatch found with wide_article_preview-0.png. I looked at the before and after images and they looked identical to me. I suspect this was just a false positive?

Let me know if you have any questions or would like me to make any changes. Thanks!

[mturoci]

Good job @dbranley! A few comments + please add a new python example with a background image graphics card as well.

[mturoci]

Let's replace this unit test with an entry in widgets section. The best place would probably be Widgets > Plots > Graphics or Widgets > Plots > AI (whatever makes more sense to you) and include 2 variations, a simple graphic and a simple graphic with a background image. That way, our documentation will be better and this functionality will be picked up by our visual regression testing suite.

Feel free to reach out in case you need help with any of the above.

[dbranley]

Hi @mturoci, question for you about this. Would it be better to add this to Guide > Graphics instead since that page already explains the ui.graphics_card? I could add a new section at the bottom to explain the new background image feature. That seems like a more logical place than Widgets > Plots, but maybe I'm overlooking something?

[mturoci]

Guide > Graphics isn't picked up by visual regression testing suite + is more of a docs on how to work with graphics rather than how to display a widget and all its variations. The widget page should definitely have a link to the graphics guide though.

[dbranley]

@mturoci, As requested, I removed the new test. For the documentation, I decided to add the new Graphics section at Widgets > Content > Graphics, just after the section for Image. The graphics stuff didn't seem to fit with the other information under Widgets > Plots, but it seemed to go well with the information under Widgets > Content. Let me know if you disagree and I can move it to wherever you prefer. I built the documentation and here's a short video showing the new page:

issue1817_doc-widget.mov

[mturoci]

Is this wrapper necessary? Wouldn't just adding style to an existing wrapper be enough?

[dbranley]

@mturoci, that was my original idea too. But the problem is that the existing div is rendered with an absolute positioning offset of 15px around all sides. So when I put the background-image on that div, it does not cover the entire card. Here's a screen show showing what that looks like:

I tried various things to adjust the styling of the div -- I tried setting margin and padding, but nothing looked right. Plus I was concerned about backwards compatibility issues when changing the styling of the main div with the graphics. As a result, I decided to add a new parent div. If you can think of another approach I'm overlooking, let me know!

[mturoci]

I see. I think the offset is fine though because even though the image in your current implementation would take up the whole card, the outer 15px wouldn't be reachable by graphics render, if I understood it correctly, which would be a problem especially when a user would want to highlight a certain part of the image with a rectangle for example.

[dbranley]

Thanks @mturoci. So just to be clear, you are saying to remove the new parent div and instead put the background image on the existing div, which is defined with the 15px offset - right? This means the background image will not cover the entire card, as you can see in this screen shot. Please confirm that I'm understanding correctly. Thanks!!

[mturoci]

Correct :)

[dbranley]

@mturoci I made the changes as suggested. Removed the parent div and added the background-image stuff to the existing div. Here's a screen shot of the custom example I've been using:

[mturoci]

I looked at the before and after images and they looked identical to me. I suspect this was just a false positive?

Yes. The tool is very sensitive, which is fine.

[dbranley]

@mturoci I think I've made all the changes you requested. I added a new Python example called graphics_background.py and rebuilt the docs. Here's a screen shot of that:

I also re-ran all the unit tests and they still pass:

Let me know if you have any questions or see something else I need to change. I'll jump on the changes in the Table Filter Count PR next. Thanks!

[mturoci]

LGTM :)