-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Preview Images in a Modal #2101
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.
Very nice 👍
Maybe center the image?
"spree/admin/shared/modal", | ||
target: "modal-image-#{image.id}", | ||
title: image.alt, | ||
content: image_tag(image.attachment.url(:large)) |
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.
Just my personal taste, but wouldn’t it be nice to wrap the image in div
that centers the 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.
Great change! I agree with @tvdeyen that it would be nice to center that image. Maybe centering is not needed for other kind of content that can land in the modal.
Updated to include centering of the images. This will likely not happen on normal stores as the images we're selecting to go in this modal are larger than the modal itself but I'm happy to provide a fallback for smaller images. |
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.
Nice
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.
The image inside the modal inherits the border style from tbody tr img
. We should remove the border from the image in the table in a separate PR. WDYT?
This prevents us from being able to effectively use Bootstrap's modal component and is generally bad practice.
The modals use a `button.close` element to show the X. Our override here causes that button to have incorrect hover states. This is a hint that we'll need to migrate everything over to use Bootstrap's class names so we can avoid extensions like this.
This will allow developers to easily add modals to the admin area where necessary.
Now that we have Bootstrap we don't need to send admins to another page just to view the larger version of the image.
@tvdeyen a separate PR makes sense to me. Unfortunately overriding the value here is really hard. I tried adding: [id^="modal-image-"] img {
// Images in tables automatically have a border around them but we don't want
// that in our modal.
border: none;
} But of course it's not strong enough to override this incredible selector: table tbody tr.even td img {
border: 1px solid #e7f0fa;
} |
Additionally this PR adds a shared modal partial for the admin and instructions on its usage to the style guide.