Skip to content
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

Fixes Justify layout #2766

Merged
merged 6 commits into from
Dec 10, 2024
Merged

Fixes Justify layout #2766

merged 6 commits into from
Dec 10, 2024

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Nov 28, 2024

Some mistakes were made...

@ildyria ildyria added the Review: easy Easy review expected: probably just need a quick to go through. label Nov 28, 2024
@ildyria ildyria added this to the 6.1.3 milestone Nov 28, 2024
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.60%. Comparing base (72ecd9a) to head (f55ee6d).
Report is 13 commits behind head on master.

Additional details and impacted files

}

// console.log("baseWidth - paddingLeftRight - scrollBarWidth")
// console.log(baseWidth - paddingLeftRight - scrollBarWidth)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log debug code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahaha :)

@Gendra13
Copy link
Contributor

Gendra13 commented Nov 30, 2024

@ildyria Thank you.
I did run a few test and indeed the gap mentioned in point 1 of #2765 is gone.

However, I am still experiencing the behavior mentioned in point 2, that the layout changes slightly (= the right gap becomes bigger) when the album is redrawn like after switching layout or selecting an image. This is even more pronounced when the timeline is activated.

I used the remains of the debug code 😉 to figure out why and I could see, that the values for the baseWidth as well as el.Width are different between the first draw (opening the page) and the consecutive redraws (changing something on the open page).
On the first draw baseWidth and el.Width are both equal to the whole browser windows (window.innerWidth), in my case 2560px.
However, on the redraws baseWidth is 2543px (~Screen minus Scrollbar), el.Width is 2506px (~Screen minus Scrollbar minus 2x padding). Even now, when the scrollbar isn’t accounted for twice, the result of GetWidth() is smaller then on the first draw by 18px (2543px vs 2506px).
This looks like one padding is still accounted twice. This would also explain why the effect is bigger with the timeline, when there is a larger padding. In my case there was a 51px difference as a result for GetWidth() (2457px vs 2406px).

One way I could easily solve this was by performing all calculations based on the browser window instead of the element sizes that change during page rendering:

-        const baseWidth = Math.floor(baseElem.offsetWidth - padding);
-        const widthEl = parseInt(getComputedStyle(el).width);
+        const baseWidth = Math.floor(window.innerWidth - padding);
+        const widthEl = window.innerWidth;

With that change both the normal as well the timeline view work without problems.

However, I am not so deep into the code to judge if it’s a good idea to base everything on the browser windows size. Maybe you have a better idea or would this be a viable Option?

@Gendra13
Copy link
Contributor

Gendra13 commented Dec 4, 2024

👍 I've tested the new changes and can confirm it now works as expected.
Thanks a lot.

@ildyria ildyria merged commit 4d50719 into master Dec 10, 2024
52 checks passed
@ildyria ildyria deleted the more-fixes-layout branch December 10, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants