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

AppTile adjustments #2424

Merged
merged 9 commits into from
Nov 20, 2024
Merged

AppTile adjustments #2424

merged 9 commits into from
Nov 20, 2024

Conversation

jordankoschei-okta
Copy link
Contributor

This PR fixes a height issue with AppTile and adds the new compact variant required by End User Dashboard.

@jordankoschei-okta jordankoschei-okta requested a review from a team as a code owner November 20, 2024 18:32
variant === "compact"
? APP_TILE_COMPACT_IMAGE_HEIGHT
: APP_TILE_IMAGE_HEIGHT,
width: variant === "compact" ? "100%" : "auto",
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are setting a height on these images, we probably should not set a width. It can skew the images

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens in the event someone uses an img that is much wider than it is tall? Will it overflow the container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it looks in practice, with multiple aspect ratios:

Screenshot 2024-11-20 at 3 00 24 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to tell with these types of placeholder images. Have you tried it with more realistic images? My hunch is we get some interesting results. Mainly that the image would be very skewed in the 'wide' case

Copy link
Contributor

Choose a reason for hiding this comment

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

But, I could be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-11-20 at 4 49 29 PM

I was able to manually verify that each image maintains the aspect ratio of its full-size equivalent

@oktapp-aperture-okta oktapp-aperture-okta bot merged commit 028013d into main Nov 20, 2024
1 check passed
@oktapp-aperture-okta oktapp-aperture-okta bot deleted the jk-apptile-height branch November 20, 2024 22:26
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.

2 participants