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

"Lazy" resize rows & columns with a guide line #4396

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

azmy60
Copy link
Contributor

@azmy60 azmy60 commented Feb 13, 2024

Added an option to "lazy" resize the columns and rows, similar to google sheets resizing. This could help reducing the DOM rendering jobs, especially for huge tables!

const table = new TabulatorFull("#example-table", {
	resizableRows: true,
	resizableRowGuide: true,
	resizableColumnGuide: true,
        columns: ...,
        data: ...,
}

Animation4


Also fixed a bug where the resizer doesn't align with mouse if the browser is zoomed in or scaled up:

Animation4

Fixed it by using e.clientX/e.clientY instead of e.screenX/e.screenY.

Animation4

refs #4313

@olifolkerd
Copy link
Owner

Could you explain the usage case a bit more for this one?

The virtual renderer means that column resizing isn't a taxing exercise as only the visible rows are actually resized, the others update as they are drawn.

I would like to understand the reasons behind this update a bit more, what actual issue is it addressing or is it just a cosmetic alternative?

Cheers

Oli

@azmy60
Copy link
Contributor Author

azmy60 commented Feb 13, 2024

I think the virtual renderer is great and using it alone with big tables is not an issue. It's really just to boost the performance because the DOM is being updated once instead of multiple times. It's a nice thing to have and heavily inspired by google sheets and ms. excel.

@rathboma
Copy link
Contributor

@olifolkerd this emulates behavior present in Google Sheets, we've found that in practice not updating the cell DOM continually during resizing has a massive positive performance impact for tables with very large cell contents.

@olifolkerd
Copy link
Owner

Hey @azmy60

Thanks for the additional details,

That makes a lot of sense, and good work putting it on a separate option to allow users to choose.

I will look at including this on the 5.7 release next month

Cheers

Oli :)

@olifolkerd
Copy link
Owner

@azmy60

There appears to be a change to the row height calculation function in this that has nothing to do with this functionality. It will also break a lot of other table layout maths.

Could you please remove any unrelated changes to this feature from the PR, or explain why you have change it, as it breaks many other parts of the table

Cheers

Oli :)

@olifolkerd olifolkerd added the PR Needs Work The PR is a good idea, but needs some updates before it can be merged label Feb 18, 2024
@azmy60
Copy link
Contributor Author

azmy60 commented Feb 18, 2024

Hi @olifolkerd , Thanks for the comment!

Yes, I made a little refactor to the row class so we could have min/max height calculation by its own methods: calcMinHeight and calcMaxHeight. These are used for positioning the resize line.

I couldn't find the min/max height of row to be accessible from outside of the Row class, so I extracted them to their own methods, but I'm not sure which part of it I'm missing or if there's a better solution so let me know! :)

@olifolkerd
Copy link
Owner

Hey @azmy60

Thanks for the clarification, i had totally misinterpreted the changes there, that makes a lot of sense.

Cheers

Oli :)

@olifolkerd olifolkerd changed the base branch from master to 6.0 March 11, 2024 14:03
@olifolkerd
Copy link
Owner

olifolkerd commented Mar 11, 2024

Hey @azmy60

Thanks for the cool new functionality, the column resize is very smooth, i will merge it into the 6.0 release.

Just a couple of notes:

  • The usual issue that keeps cropping up, you should only declare vars at the top of the function
  • To make things a bit easier on me, when you add new SCSS, can you please add it in the correct place in the selector tree rather than at the bottom of the SCSS file, it make it much easier to find classes associated with a row or column in the CSS if they are located inside the row or column selector in the SCSS, for example in this case the elements are contained inside the table element so that is where the selectors should sit. The only classes that sit on the tree are for modules like the print module where it is building an entire new table that is not part of the Tabulator, so it therefore is appended to the end of the SCSS file. I have moved the guide classes to just below all of the other resize classes
  • The row resize functionality was broken, there were scope issues in the resize function that caused all row resize functionality to break and when that was fixed the guide bar didn't move correctly either. I have fixed the issues now, but could i ask that you please make sure that proposed functionality is working before the PR is submitted.

If you are on the discord, and you havnt already, drop me a message and i will give you the contributor badge :)

Cheers

Oli :)

@olifolkerd olifolkerd merged commit 00d81c3 into olifolkerd:6.0 Mar 11, 2024
2 of 3 checks passed
@azmy60
Copy link
Contributor Author

azmy60 commented Mar 12, 2024

Hi @olifolkerd Thanks for the feedbacks! I always test the codes before submitting it though it's really my fault if something breaks because that could've been something I've missed 🙏.

Feel free to ping me if you need me to fix my PRs next time : )

Also just reached out to you on discord. The username is the same as my GitHub username.

@olifolkerd
Copy link
Owner

All good, was a couple of quick one line fixes, only took a couple of mins.

Great bit of functionality, love the idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Needs Work The PR is a good idea, but needs some updates before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants