fix #1035 : Column alignment when zoom font only settings in browser

[hexaltation]

fix #1035

Use rem value instead of fixed pixel where needed.
Removed inline style 52px for .gridview_data_row_num

[fflorent]

It works much better, thanks!

For the other reviewers, the "zoom text only" feature is used for people with visual deficiencies, so that improves the accessibility.

Here is a preview (you must use Firefox, Chrom* browsers don't have such feature):

Without the patch

recording-orig-behavior.mp4

With the patch

recording.mp4

[paulfitz]

(apologies, I'm going to use this PR as a guinea pig to examine exactly why our machinery for deploying previews of PRs doesn't function for PRs from forks)

[berhalak]

Hi @hexaltation.

Can you extend the comment here with the math behind it? How 4 * 3.25 * root font size equals 52px ?

[hexaltation]

Hello @berhalak

I pushed a more (I hope) explicit comment.

The problem is I don't know why a factor of 3.25 have to be applied, except that 52px is the working value in InitialOffset and MiddleOffset.

May be it's better to get rid of ROW_NUMBER_WIDTH_PX and make the 3.25 factor appear by magic in Offsets formulas.

const initialOffset = -this.frozenWidth() - 3.25 * ROW_NUMBER_WIDTH + this.width() - revealWidth - PLUS_WIDTH;

const middleOffset = -this.frozenWidth() - 3.25 * ROW_NUMBER_WIDTH_PX + this.width() / 2;

Let me know

[dsagal]

One note I'd like to add is to be sure to test this with frozen columns.

[hexaltation]

(apologies, I'm going to use this PR as a guinea pig to examine exactly why our machinery for deploying previews of PRs doesn't function for PRs from forks)

If it can improve this workflow it's a pleasure :)

[paulfitz]

(apologies, I'm going to use this PR as a guinea pig to examine exactly why our machinery for deploying previews of PRs doesn't function for PRs from forks)

If it can improve this workflow it's a pleasure :)

Update: we know what the issue is, and making it works needs some rejiggering to the workflow that @SleepyLeslie will be looking at.

[berhalak]

I'm still confused a little bit :). In this line:

// as 1rem=16px -> 3.25 * 4 * 16 = 52px when no text zoom is applied

• 1rem equals to 13px in Grist, not 16px
• The math is wrong, even for 16px, the expression equals to 208px

I think it will be hard to do the math here properly, as the effect you want to achieve is a mix of font-size width and absolute width zooming. In your video, font is made bigger, but columns widths are not changed. Only the row number div is scaled. The math used for scrolling or calculating frozen offset doesn't assume that, it looks like a coincident that it works. I think we have some sanity checks and some values are trimmed, and it somehow works.

I think that the proper behavior is in the video below:

zoom-font.mp4

And this can be easily achived by replacing all 4rem and 4em sizes with 52px in GridView.css.

[hexaltation]

@berhalak I've made the needed changes according to your suggestions.
Can you re-review it ?

[hexaltation]

One note I'd like to add is to be sure to test this with frozen columns.

👍

[github-actions]

Deployed commit 3678ab2232d61f81538b4884ebd66a6b81302e3b as https://grist-hexaltation-grist-core-1035-align-columns-when-zoom.fly.dev (until 2024-08-22T10:04:23.218Z)

[berhalak]

Thanks @hexaltation !