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

feat: ui.image #670

Merged
merged 7 commits into from
Aug 5, 2024
Merged

feat: ui.image #670

merged 7 commits into from
Aug 5, 2024

Conversation

ethanalvizo
Copy link
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
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
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
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.

plugins/ui/src/deephaven/ui/components/image.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/components/image.py Outdated Show resolved Hide resolved
plugins/ui/src/deephaven/ui/components/image.py Outdated Show resolved Hide resolved
AkshatJawne
AkshatJawne previously approved these changes Jul 29, 2024
Copy link
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

@ethanalvizo ethanalvizo merged commit 874ba97 into deephaven:main Aug 5, 2024
14 checks passed
@mofojed mofojed mentioned this pull request Sep 16, 2024
88 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