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

Fixes #1165 by making a missing alt tag a console warning #1167

Merged
merged 3 commits into from
May 2, 2022

Conversation

benjaminsehl
Copy link
Member

Fixes #1165

Description

When you use <ModelViewer /> or <MediaFile /> today, and the 3D asset you're providing has not yet had an alt tag set in the Shopify admin, Hydrogen will break. As this is not something within our control, and any ecommerce merchandiser could upload a 3D model and forget to add an alt tag, we should not throw an error and instead (I propose) provide a console warning that this is missing.

Additional context

  • Is this the right way to go about this?
  • I noticed on line 124 there was a type defined: type PropsWeControl = 'src' | 'alt' | 'poster';, should that change too?

Before submitting the PR, please make sure you do the following:

  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
  • Update docs in this repository according to your change
  • Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

@github-actions

This comment has been minimized.

@benjaminsehl benjaminsehl requested a review from frehner May 2, 2022 19:21
@frehner
Copy link
Contributor

frehner commented May 2, 2022

I noticed on line 124 there was a type defined: type PropsWeControl = 'src' | 'alt' | 'poster';, should that change too?

Yeah, good catch; let's remove alt there too!

Looks like the test failed just due to prettier; if you run yarn format it should fix it, and looks good to me after that!

@benjaminsehl benjaminsehl requested a review from jplhomer May 2, 2022 19:22
Copy link
Contributor

@frehner frehner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE

@benjaminsehl benjaminsehl merged commit 0a5ac1c into v1.x-2022-07 May 2, 2022
@benjaminsehl benjaminsehl deleted the bs/MediaFile-alt-fix branch May 2, 2022 20:18
blittle added a commit that referenced this pull request May 4, 2022
* v1.x-2022-07: (95 commits)
  [ci] release v1.x-2022-07 (#1170)
  Try ignoring hello-world to see if it will get bumped
  Don't consider examples part of the workspace (#1202)
  Fix headers on oxygen (#1201)
  Add bot user agents for Seoradar and Adresults, resolves #1199 (#1200)
  Fix changeset
  updates to docker deploy documentation to resolve run issues (#1196)
  Upgrade body-parser (#1162)
  Fix path for deployments
  Adds ability to add more than one cookie per response (#1161)
  Move Demo Store to templates folder (#1132)
  Avoid additional div element (#1191)
  Whoops this should only be patch
  Adds preconnect <link> for CDN (#1160)
  Bump ejs from 3.1.6 to 3.1.7 (#1147)
  Fix scroll restoration when server props are changed (#1152)
  Typo
  Fixes #1165 by making a missing alt tag a console warning (#1167)
  Remove concurrency directive for Oxygen deployments
  Fix hydrogen-ui dev and build issues (#1169)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] <MediaFile> breaks if no alt text is assigned to a 3D Model
3 participants