LPS-124271 Resolve computed image sized based on real media queries #3234
Conversation
|
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
Instead of using viewports [min, max] sizes, we reuse existing globalContext to check if any image media query matches, which emulates CSS behavior and should produce the exact same result. There is one situation where this process does not behave like CSS: window resizing. If user changes window size without closing/opening the sidebar panel, it won't update automatically. I've intentionally ignored this situation because it would lead to a lot of extra computation (a resize listener is not cheap) and it does work if our viewport size selector is used.
p2kmgcl
added
the
🔍 Frontend Review Needed
|
ci:test:sf |
|
ci:test:relevant |
| type === EDITABLE_TYPES.image && handleImageSizeChanged | ||
| type === EDITABLE_TYPES.image | ||
| ? handleImageSizeChanged | ||
| : null |
There was a problem hiding this comment.
propTypes were failing?
There was a problem hiding this comment.
More or less. If you pass false to an optional value it cries, but if you pass null it's fine 😆
liferay-continuous-integration
added
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: bg-im-124271 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#4871 |
|
LGTM, btw is this is a requirement of the story? Just wondering if we could just set the resolution based on the width from the viewports instead of relying on the current size of the window which would make things a bit simpler |
Sadly it didn't because:
But yes, this is terribly complex and I would prefer not to include it |
victorg1991
added
✅ Ready to Merge
|
Ship it! |
|
Maybe some poshi tests need adjustement though |
|
Hi @p2kmgcl and @victorg1991, the Auto is not shown in Resolution field of Image fragment in non-desktop viewport by default when use direct Image Source. Could you have a look at? |
|
On my way |
|
@Tim-Cao I found to small issues, but the resolution is displayed correctly. Can you check with these changes? |
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: bg-im-124271 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#2889 |
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#96949 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#3959 |
|
@p2kmgcl, works as expected |
No description provided.