-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add lazy loading threshold for ImageEx control #3483
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
Conversation
merge original source
merge original source
merge original source
Thanks h82258652 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
@michael-hawker Done, can you review again? |
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.
Couple of questions. Also, does this mean we can close #3255 or are these independent?
@@ -80,7 +82,7 @@ public ImageExBase() | |||
{ | |||
LockObj = new object(); | |||
|
|||
EffectiveViewportChanged += ImageExBase_EffectiveViewportChanged; | |||
LayoutUpdated += ImageExBase_LayoutUpdated; |
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.
Don't we need to do both still?
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.
@michael-hawker Don't need, issue #3258 is occurred by register EffectiveViewportChanged
while the control host in a Viewbox
, it is a framework bug.
@@ -6,6 +6,7 @@ | |||
<Setter Property="Background" Value="Transparent" /> | |||
<Setter Property="Foreground" Value="{ThemeResource ApplicationForegroundThemeBrush}" /> | |||
<Setter Property="IsTabStop" Value="False" /> | |||
<Setter Property="LazyLoadingThreshold" Value="300" /> |
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.
Is there a reason you picked 300?
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.
@michael-hawker About this I think a lot. I searched google and it tells me Chromium will calculate this by the screen size and network type. And then I searched the most widely used JavaScript image lazy loading library, got this. This lib use 300px as default. I think UWP is mostly used on a PC, not a mobile device. The network should be not bad, 300px should be suitable for most situations.
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
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.
Caught this one last thing I think. Then we should be good.
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Fixes
Add lazy loading threshold for
ImageEx
control and change the lazy loading implement that will be also fixed #3258 .PR Type
What kind of change does this PR introduce?
What is the current behavior?
If enable lazy loading, only the ImageEx entered the viewport, the source will start to load.
What is the new behavior?
Give a threshold for triggering the lazy loading behavior, the default value is 300 px.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information