Page MenuHomePhabricator

Move Macro image height/width to CSS
ClosedPublic

Authored by chad on Mar 11 2015, 9:35 PM.
Tags
None
Referenced Files
F14058490: D12045.diff
Sun, Nov 17, 12:37 PM
F14037664: D12045.diff
Sun, Nov 10, 6:07 PM
F13996967: D12045.id28992.diff
Thu, Oct 24, 1:27 AM
F13986121: D12045.id28990.diff
Mon, Oct 21, 12:45 AM
F13969051: D12045.id29000.diff
Oct 17 2024, 12:14 AM
Unknown Object (File)
Sep 6 2024, 1:26 AM
Unknown Object (File)
Sep 3 2024, 5:01 AM
Unknown Object (File)
Aug 27 2024, 4:50 AM
Subscribers

Details

Summary

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

Test Plan

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

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad retitled this revision from to Move Macro image height/width to CSS.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.

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.

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.

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.

chad edited edge metadata.
  • Second Try

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?

This seems to work for macros on scrollbars, but fullsized remarkup images and memes need to add height/width support.

  • Adds height/width support to files size=full

Haha, given that last image macro, I'll defer to @epriestley... :D

epriestley edited edge metadata.
epriestley added inline comments.
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.

This revision is now accepted and ready to land.Mar 11 2015, 11:38 PM

(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.)

src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
99–100

[done]

src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php
115–116

done

webroot/rsrc/css/core/remarkup.css
493–494

Seems to work, I had it in there when they were still set with style originally.

[done]

chad edited edge metadata.
  • Second Try
  • derp
  • Adds height/width support to files size=full
  • celery
  • Update per comments
This revision was automatically updated to reflect the committed changes.