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

[BUG] Duplicated HTML id due to Hydrogen <Image /> #624

Closed
davidhousedev opened this issue Sep 8, 2022 · 4 comments
Closed

[BUG] Duplicated HTML id due to Hydrogen <Image /> #624

davidhousedev opened this issue Sep 8, 2022 · 4 comments
Labels
Bug Something isn't working @shopify/hydrogen-react

Comments

@davidhousedev
Copy link
Contributor

Describe the bug

In our designs on Product Details Page, we have a series of image thumbnails with one image that's "selected" and displayed large. We are using the Hydrogen <Image /> component which has the default behavior of setting the HTML id to the underlying id of the image. The result is that we have two <img>s on the page with the same id.

To Reproduce

  1. Render a Hydrogen page with two <Image /> components, each that renders the same image
  2. Inspect the rendered DOM
  3. Notice that there are two <img>s that share the same id

Expected behaviour

I would not expect the Hydrogen <Image /> to set an id. If it did, I'd expect the underlying behavior to guarantee uniqueness via useId or the like.

Screenshots

image

image

Additional context
Add any other context about the problem here. eg.

Hydrogen 1.2.0

@davidhousedev davidhousedev added the Bug Something isn't working label Sep 8, 2022
@services-db

This comment was marked as off-topic.

@frehner
Copy link
Contributor

frehner commented Sep 8, 2022

Yeah, I think this makes sense. That was on there before I started working on components, so I don't have context on the reasoning it was added in the first place. But I think it should be removed if no one has a concrete reason for adding it; I'll ask around.

@elisenyang elisenyang transferred this issue from Shopify/hydrogen-v1 Oct 25, 2022
@aminpaks
Copy link

aminpaks commented Nov 5, 2022

I started looking into this, I think there is an underlying design question about setting an ID or not to the HTML image tag. The <Image /> component has no wrapper, should we not set the ID for the HTML image?

Not setting an ID at all may not be ideal for all use-cases. Currently we set the ID coming in data.id from props, and I think it is already protected by not being at the root level. It should be the consumer's responsibility to provide a unique ID, or skip it.

Thoughts @frehner, @davidhousedev?

Also I noticed that we're setting it to an empty string '' if not provided, I think we shouldn't. If it's undefined, it should stay as is.

@frehner
Copy link
Contributor

frehner commented Nov 7, 2022

should we not set the ID for the HTML image? Not setting an ID at all may not be ideal for all use-cases.

I genuinely don't know; is there any value in us always setting an ID for it? My initial impression is that I don't think there is, but I could be missing some important reason as to why we would want to do it.

@frehner frehner transferred this issue from Shopify/hydrogen-react Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working @shopify/hydrogen-react
Projects
None yet
Development

No branches or pull requests

4 participants