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

change frozen style like google spreadsheet #483

Merged
merged 6 commits into from
Feb 23, 2022

Conversation

xianzhi3
Copy link
Contributor

@xianzhi3 xianzhi3 commented Feb 7, 2022

Hi, @ndrsn I changed the frozen style like google spreadsheet.
You can have a test at the link below.
https://90zfd.csb.app/demo.html

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 8, 2022

Hi @xianzhi3 thanks for this. I believe I've discovered two bugs/quirks. For now I can only reproduce the one below here:

CleanShot 2022-02-07 at 23 30 23

I'll see if I can narrow down the other later this week.

@xianzhi3
Copy link
Contributor Author

xianzhi3 commented Feb 9, 2022

@ndrsn I fixed the quirks bug

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 17, 2022

Hi! Great! I managed to reproduce the other one. The recording is a bit long, because I'm not sure which parts of it are relevant, but here is the weird behavior:

CleanShot 2022-02-16 at 22 28 55

@xianzhi3
Copy link
Contributor Author

@ndrsn I fixed the recording issue. You can test it at the same link.

@mdebrauw
Copy link
Contributor

Hi @xianzhi3 I (visually) tested this too using the link you provided.

Still seeing some weird behaviour, sudden and fast mouse movements seem to make the grid disappear for a moment.

CleanShot 2022-02-18 at 13 58 24

@xianzhi3
Copy link
Contributor Author

It works well in my end. What is your computer OS?

@mdebrauw
Copy link
Contributor

mdebrauw commented Feb 18, 2022

Chrome Version 98.0.4758.80 (Official Build) (arm64) on macOs Monetery (12.0)

Also tests on Safari (Version 15.0 (17612.1.29.5))

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 19, 2022

I can reproduce this as well — see the white screen near the end of the animation. Only once I move the mouse again the grid re-renders:

CleanShot 2022-02-19 at 15 48 34

(I am on macOS as well, for the record)

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 19, 2022

Another one:

CleanShot 2022-02-19 at 16 08 05

@hangxingliu
Copy link
Contributor

hangxingliu commented Feb 19, 2022

I think the key to reproduce this bug is:

You drag the frozen marker and let your mouse leave the browser, then backed to the grid.

And the grid is empty(only has white background) until you move your mouse to trigger next render.

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 20, 2022

You drag the frozen marker and let your mouse leave the browser, then backed to the grid.

Maybe that's one edge-case as well, but in this example I don't leave the browser boundaries with my cursor:

CleanShot 2022-02-19 at 22 27 13

And here's another (longer) recording, taken after I've left this tab open for a few hours (not sure if that's relevant, but my perception is that the issue becomes more frequent in this recording):

CleanShot 2022-02-20 at 00 07 36

(Apologies for the million open tabs, I am seeking professional help)

@hangxingliu
Copy link
Contributor

@ndrsn can you have a test on this demo: https://9l3dgo.csb.app/

the changes between the demo and the latest commit of this PR are here:

Screenshot 2022-02-21 at 04 52 01

(btw, you may can try the extension named OneTab or Better OneTab for organizing the tabs)

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 21, 2022

@hangxingliu that seemed to work, thanks! But why, though? The behaviour seemed a little flaky/inconsistent, and it's not clear to me how this fix explains what was happening.

I'll be happy to merge this (including your fix), and don't want to cause anyone more work, but if you or @xianzhi3 can explain what the bug was and why this change is the fix we need, I'll be a little more confident in merging :-)

@hangxingliu
Copy link
Contributor

@ndrsn I am not 100 percent sure my guess is correct. If I need to explain to you, I need another test for this bug. But I am working on the refactoring of self.selections for performance purpose (On another branch with unstaged changes). I can test it again and tell you after I finished the util functions of it.

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 21, 2022

@hangxingliu That sounds reasonable! Do you suggest we merge this now? If this is part of the code you guys will be touching anyway, I trust any lingering bugs will be ironed out anyway :-)

@hangxingliu
Copy link
Contributor

@ndrsn The investigation of this bug is almost done, I will write it in the Slack channel one message by one message. I think you can merge this PR and the patch after you read them.

@ndrsn
Copy link
Collaborator

ndrsn commented Feb 22, 2022

Alright! @xianzhi3 can you fix the merge conflicts? Then I'll merge right after.

Update: noticed some things in the diff which I can't quite link to the frozen style change proposed — not sure if they're GitHub messing up the diff (it does that some times), or if they're unrelated changes that somehow snuck into the PR. @xianzhi3 , do you know? Perhaps a rebase is in order?

@ndrsn ndrsn merged commit c82a5d3 into TonyGermaneri:master Feb 23, 2022
@ndrsn
Copy link
Collaborator

ndrsn commented Feb 23, 2022

Great work @xianzhi3 and @hangxingliu, thanks a lot!

ndrsn added a commit to ndrsn/canvas-datagrid that referenced this pull request Feb 23, 2022
* change frozen style like google spreadsheet

* fix quirks bug

* fix recording issue

* Fix unhide indicator test

* Really fix it this time (?)

Co-authored-by: Jona Andersen <jona@ndrsn.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants