e-oj / Magic-Grid

A simple, lightweight Javascript library for dynamic grid layouts.
https://www.npmjs.com/package/magic-grid
MIT License
3.14k stars 144 forks source link

Fix colWidth calculation issue #25

Open amytych opened 5 years ago

amytych commented 5 years ago

There are sub-pixel differences between col widths returned by different browsers, when the width is not defined in absolute pixel values, e.g. a percentage. In Firefox, for example, it causes a three column layout to be rendered as a two column layout.

See this fiddle forked from your example in the README: https://jsfiddle.net/avwgdurp/ When you resize the preview container you notice in Firefox the layout jumps between 2 and 3 columns, whereas in Chrome there are always 3 columns rendered.

Chrome: chrome

Firefox: firefox

This change floors the col width value to the whole pixel, ensuring their combined width is never wider than the container.

e-oj commented 5 years ago

Unfortunately, There's no real fix for this issue due to the lack of standardization around sub-pixel rendering. Browsers handle sub-pixels in different ways; some round up, some round down, and some do both.

If we round down the value returned by colWidth, we might get overlapping elements in browsers that round up sub-pixels, and if we round up colWidth, we risk creating columns that are wider than their components.

Check out these articles for more info on this issue:

amytych commented 5 years ago

I see, I'll close it then. Thanks for the response and the articles!

amytych commented 5 years ago

I tried one more idea to solve this issue. Please let me know what you think about it, I explained it in the commit message.

e-oj commented 5 years ago

Have you tested this?

amytych commented 5 years ago

Yes, I did, please check this fiddle based on the version of MagicGrid from this PR. When you resize the window in Firefox, you can see that the layout always keeps 3 columns, instead of jumping between 2 and 3 columns like in the original one. https://jsfiddle.net/txyojrq3/

image