This makes macros and memes grow to 100% of their container at most, instead of showing a scrollbar. This is useful for overly large macros, smaller spaces like Feed and Conpherences, and Inline Comments. Fixes T7528
Details
Details
- Reviewers
epriestley btrahan - Maniphest Tasks
- T7528: Remarkup Images (macros, etc) should scale to space
- Commits
- Restricted Diffusion Commit
rP6a036f32b249: Move Macro image height/width to CSS
Tested a very large macro, a very large meme, and a very very tiny macro. It looks like memes get cached though, unsure if we should clean them up or just leave them
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I think there's issues calculating he height (and width) of the containing element if the interior elements like images don't have a height set?
This can be a problem when e.g. trying to autoscroll the conpherence message window... If interior elements don't have specific heights / widths then we can't scroll enough until we figure out what the real height is....
Hope that makes sense / maybe this is obviated by other changes. We definitely had to do work to add image dimensions to files (and render them consistently) for this (and other) reasons though.
Comment Actions
Hrm...
So what are the options here? This works correctly, rendering wise, but not scrolling wise. Sounds like any "correct" solution would be JS based.
Also, looks like meme's don't post height/width, so that'd need fixing anyways.
Comment Actions
what if we
- set width and height to known values (may be huge) inline
- set max-width and max-height to fancy, correct CSS values
...then I think any javascript should calculate "too much" space over too little. For now, functionally that's okay since we are always scrolling to the bottom as far as I know. I think the CSS part will work correctly but I don't know for sure.
Comment Actions
This seems to work in practice. The height and width are moved to img attributes instead of CSS styles. Not sure how to solve this in the macro sense, but that's not a regression at least.
| src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php | ||
|---|---|---|
| 115–116 | do I need to set these to a default? do we have old macros without dimensions? | |
Comment Actions
This seems to work for macros on scrollbars, but fullsized remarkup images and memes need to add height/width support.
| src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php | ||
|---|---|---|
| 99–100 | You should be able to call $file->getImageHeight() and $file->getImageWidth() directly. | |
| src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php | ||
| 115–116 | Here too. | |
| webroot/rsrc/css/core/remarkup.css | ||
| 493–494 | Do these not work without !important? I'd expect them not to require !important. | |
Comment Actions
(Old files may not have dimensions, but the getWidth/Height calls will return null and phutil_tag() should decline to render the attribute when the value is null, so this should work fine for them.)
Comment Actions
- Second Try
- derp
- Adds height/width support to files size=full
- celery
- Update per comments