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

Disable text eliding by default #451

Merged
merged 5 commits into from
Nov 2, 2022
Merged

Conversation

ibdafna
Copy link
Member

@ibdafna ibdafna commented Oct 31, 2022

Addresses #448

@fcollonval fcollonval added enhancement New feature or request performance Addresses performance labels Oct 31, 2022
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @ibdafna could you run yarn api to make the CI check happy 😉 ?

Does it solves the issue or should we let the issue opened?

@krassowski
Copy link
Member

I would say that this does not fix the issue, since we can make vast algorithmic improvements (and even keep eliding enabled by default if we do that). While, it certainly helps to be able to disable eliding, ideally it could be disabled/enabled on per-column basis.

@ibdafna
Copy link
Member Author

ibdafna commented Oct 31, 2022

@krassowski This is a good start, and it does address the issue. We can open separate PRs for improving the algorithms of text eliding and wrapping. But I'd like to iterate again, there is no easy way to significantly reduce the time elide takes as it needs to make at least one call per cell to the measureText function. You can certainly enable or disable this on a per column basis by setting a custom TextRenderer for columns you want it enabled or disabled for.

@ibdafna
Copy link
Member Author

ibdafna commented Nov 1, 2022

@fcollonval yarn api doesn't generate any file changes?

@fcollonval
Copy link
Member

yarn api doesn't generate any file changes?

I don't know if you did cacc6ac manually then (thanks for that). But I think the remaining error was a typo in that commit. I fixed it. 🤞 for the CI

@fcollonval fcollonval merged commit b514471 into jupyterlab:main Nov 2, 2022
@ibdafna ibdafna deleted the elide_optional branch November 2, 2022 15:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request performance Addresses performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants