Expand image button #570
Expand image button #570
Conversation
DerMolly
force-pushed
the
feature/expandImage
branch
from
2e05dab to
2e860ce
Compare
There was a problem hiding this comment.
All in all, I'm not completely happy with the solution. :/
Some suggestions:
- as mentioned above use a lightbox instead of modals
- either fix the positioning of the enlarge button(1) or enlarge the image by clicking on it (cursor: zoom-in)
- (optional) maybe allow navigation through all images in the document when inside the lightbox (arrow keys, arrow buttons on the left and right of the enlarged image)
(1): 
src/components/markdown-renderer/replace-components/image/image-frame.tsx
Outdated
Show resolved
Hide resolved
src/components/markdown-renderer/replace-components/image/image-frame.tsx
Outdated
Show resolved
Hide resolved
| <CommonModal | ||
| show={showFullscreenImage} | ||
| onHide={() => setShowFullscreenImage(false)} | ||
| title={alt ?? ''} | ||
| closeButton={true} | ||
| size={'xl'} | ||
| icon={'picture-o'} | ||
| > | ||
| <Modal.Body> | ||
| <img alt={alt} {...props} className={'w-100'}/> | ||
| </Modal.Body> | ||
| </CommonModal> |
There was a problem hiding this comment.
I don't know but I don't like using modals for showing bigger images. You're wasting screen space by the thick modal headers and borders and you need to scroll on images in portrait format (in contrast to landscape format).
I rather would use a lightbox as many other apps and webpages do. For some examples see here: https://mdbootstrap.com/docs/jquery/javascript/lightbox/
There was a problem hiding this comment.
I don't see anything wrong with using bootstrap modals, because they are already included and provide most of the functionality. But I would remove the header, footer and padding.
ErikMichelson
added
type: feature
removed border around image in modal removed image button instead the modal is shown when the image is clicked
| <span>{alt ?? ''}</span> | ||
| </Modal.Title> | ||
| </Modal.Header> | ||
| <img alt={alt} {...props} className={'w-100'}/> |
There was a problem hiding this comment.
No bug, but an idea: If no title is present, but an alt text, then we should use the alt text as title.
src/components/markdown-renderer/replace-components/image/frame.tsx
Outdated
Show resolved
Hide resolved
changed CHANGELOG.md
DerMolly
force-pushed
the
feature/expandImage
branch
from
c79d75b to
c800b9d
Compare
|
Fix CI please |
the alt tag wasn't passed down to frame, because it's explicitly used in image-frame, but not explicitly passed down
Component/Part
markdown renderer
Description
This PR add a expand image button to bottom right corner of every image.
Steps
Related Issue(s)
#276