Skip to content

feat: ui.image#670

Merged
ethanalvizo merged 7 commits into
deephaven:mainfrom
ethanalvizo:597-image
Aug 5, 2024
Merged

feat: ui.image#670
ethanalvizo merged 7 commits into
deephaven:mainfrom
ethanalvizo:597-image

Conversation

@ethanalvizo
Copy link
Copy Markdown
Contributor

Closes #597

@ethanalvizo ethanalvizo self-assigned this Jul 25, 2024
@ethanalvizo ethanalvizo marked this pull request as ready for review July 26, 2024 13:05
@ethanalvizo ethanalvizo requested a review from mofojed July 26, 2024 13:05
Copy link
Copy Markdown
Contributor

@AkshatJawne AkshatJawne left a 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.
Copy link
Copy Markdown
Contributor

@AkshatJawne AkshatJawne Jul 26, 2024

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?

Copy link
Copy Markdown
Contributor Author

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.

Comment thread plugins/ui/src/deephaven/ui/components/image.py Outdated
Comment thread plugins/ui/src/deephaven/ui/components/image.py Outdated
Comment thread plugins/ui/src/deephaven/ui/components/image.py Outdated
@ethanalvizo ethanalvizo requested a review from AkshatJawne July 29, 2024 05:20
AkshatJawne
AkshatJawne previously approved these changes Jul 29, 2024
Copy link
Copy Markdown
Contributor

@AkshatJawne AkshatJawne left a 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

Comment thread plugins/ui/src/deephaven/ui/components/image.py Outdated
@ethanalvizo ethanalvizo merged commit 874ba97 into deephaven:main Aug 5, 2024
@mofojed mofojed mentioned this pull request Sep 16, 2024
89 tasks
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.

Add support for ui.image

3 participants