-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: ui.image #670
feat: ui.image #670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, small cleanup
grid_row_end: When used in a grid layout, specifies the ending row to span within the grid. | ||
grid_column_start: When used in a grid layout, specifies the starting column to span within the grid. | ||
grid_column_end: When used in a grid layout, specifies the ending column to span within the grid | ||
slot: A slot to place the image in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little confused about this slot
prop, tried taking a look on the Spectrum docs as well, but are there other possible values here?
If so, we could extract it to a type, and if not, is it always just slot = 'image'
(if that is the case, maybe we don't want it ex?
Also, not exactly sure by what "place the image in" means here, does it mean that it is possible for the image be placed outside of ui.image
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I couldn't find too much on this in the docs I copied what they had in their description of the prop. When I was searching up the slot attribute it seems to be used for inserting things into templates: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/slot.
I can't think of why this component wouldn't use slot = "image"
so I will not make it explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I would probably wait till @mofojed also has a peek at this before merging
Closes #597