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

Add optional alt prop to POS image #2033

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

BrittaEvansFenton
Copy link
Contributor

@BrittaEvansFenton BrittaEvansFenton commented May 29, 2024

Background

Adds alt text as an optional prop
Follows what is expected: https://github.com/Shopify/ui-api-design/tree/main/components/Image

Questions

have I missed anything?

Solution

(Describe your solution, why this approach was chosen, and what the alternatives/impacts may be)

🎩

  • ...

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@BrittaEvansFenton BrittaEvansFenton marked this pull request as ready for review May 30, 2024 19:42
Copy link
Contributor

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset directory. If the changes are user-facing and should cause a version bump, run yarn changeset to track your changes and include them in the next release CHANGELOG. If you are making simple updates to repo configuration, examples, or documentation, you do not need to add a changeset.

Comment on lines 4 to +5
src?: string;
alt?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
src?: string;
alt?: string;
/**
* The url of the image.
**/
src?: string;
/**
* Alternative text for the image.
**/
alt?: string;

Copy link
Contributor

@NathanJolly NathanJolly left a comment

Choose a reason for hiding this comment

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

I think these just need comments!

@js-goupil
Copy link
Contributor

js-goupil commented Jun 17, 2024

@BrittaEvansFenton should we merge this? Or make the changes suggested?

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.

3 participants