-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
CellEditors now render in front of the DataGrid #87
Conversation
Just realized that the changes submitted modified a lot of the formatting and styling. Does lumino have a precommit hook for styling like JupyterLab? Does lumino also follow |
Hi @kgoo124! Thanks for the pull request.
|
Hi @afshin, would you be able to elaborate a little more on the CSS rule part. I am also currently trying to get rid of the linting changes. |
I was going to propose adding: .lm-DataGrid-cellEditorOccluder {
position: absolute;
} But I just realized we don't actually have a |
Could I also propose adding a precommit hook for styling similar to JupyterLab, so that these styling issues don't become a problem for future changes? |
It would be really nice to have linting be automatic. I think the reason we haven't done it is because it's hard to configure a linter to respect and enforce the conventions already in this codebase. |
Definitely, let me know if there's anything else that needs to be done for this PR. Thanks for the quick reply! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me.
cc: @mbektasbbg (just in case there is something being overlooked)
@afshin @kgoo124 both of these elements already have CSS classes assigned to them with position set to absolute (https://github.com/jupyterlab/lumino/blob/master/packages/default-theme/style/datagrid.css#L37 and https://github.com/jupyterlab/lumino/blob/master/packages/default-theme/style/datagrid.css#L43). @kgoo124 did you import the datagrid.css into your custom editor?
|
Thanks all, merging. |
Published |
@mbektasbbg Is that CSS critical to the operation of the data grid? Should a subset of the datagrid default CSS be in the |
Yes @afshin there is some css in default-theme that is critical for the operation of datagrid. As you suggested we should bring those into datagrid package. I will try to spare some time to do that. This PR fixes only one of the issues when user doesn't import CSS from default-theme. |
@mbektasbbg, @afshin, @blink1073 that would be really helpful! Also, I think we could add some documentation specifying the |
Hi, my name is Kalen, and I am one of the Jupyter Cal Poly interns. Our team has been using the
TextCellEditor
class in attempts to create an editable DataGrid for the extension my team is working on, Tabular Data Editor. However, the cell editor renders behind theDataGrid
, and I think this is because theupdatePosition
method inCellEditor
uses the stylestop
andleft
, but theposition
isn't set. In the Tabular Data Editor, I created a class calledCSVTextCellEditor
with this as the only fix and the cell editor now renders in front of theDataGrid
.You can see the changes I made to the Tabular Data Editor extension here.
I am proposing that the
updatePosition
method in theCellEditor
class include this fix so that all cell editors can render in front of theDataGrid
immediately. Our team will be using the other cell editors in the near future and it would be redundant to extend each of these cell editors for this small fix. I would like to get some feedback and hear any other ideas.Fixes #86