Skip to content

Use custom grid element instead of VSCodeDataGrid#2990

Merged
robertbrignull merged 16 commits intomainfrom
robertbrignull/model-table-alignment
Oct 19, 2023
Merged

Use custom grid element instead of VSCodeDataGrid#2990
robertbrignull merged 16 commits intomainfrom
robertbrignull/model-table-alignment

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR removes the usage of VSCodeDataGrid from the model editor and instead applies the display: grid property manually.

The issue with VSCodeDataGrid is that it's actually each VSCodeDataGridRow that has the display: grid property. This means each row is an individual grid and getting columns to align across rows is tricky and is impossible is certain situations.

I took all the relevant styling directly from the VSCodeDataGrid family of components. If there's extra styling I missed then please say. For the "focused" condition I had to change it from a border to a background colour, because elements with display: contents cannot have borders.

One significant change is that we now have to give elements a gridRow property as well as gridColumn. This does add some extra complexity, but it also opens up the option to have elements that span multiple rows. I plan to use this feature in a followup PR to remove MultiModelColumn. My aim is that each dropdown input should be its own grid cell, instead of having all of the "kind" dropdowns into one cell in a flex column.

This screenshot shows the state before this PR (Note how the columns in the header don't match the table body):
Screenshot 2023-10-17 at 15 56 17

This screenshot shows the same view after this PR:
Screenshot 2023-10-17 at 15 52 33

And this screenshot shows how it looks when a row is "focussed" using the "reveal in editor" button (not the same as hovering, which looks identical to how it did before):
Screenshot 2023-10-17 at 16 23 01

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested review from a team as code owners October 17, 2023 16:40

const Name = styled.span`
font-family: var(--vscode-editor-font-family);
word-break: break-all;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had to add this otherwise it was breaking the grid at low screen widths. I'm not sure why this is needed now and wasn't before.

Comment thread extensions/ql-vscode/src/view/model-editor/HiddenMethodsRow.tsx
@robertbrignull
Copy link
Copy Markdown
Contributor Author

It's true we could probably fix the header column alignment by adding some invisible fixed-width element into the final cell of the header. But there's so much more that doesn't really work properly when you have lots of little grids pretending to be one big grid.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! It seems like the FAST foundation data grid has quite some accessibility features built in (like keyboard navigation), but I don't think we need that for this data grid. Most of the cells are already keyboard navigable through their inputs and focusing on a single row or cell is not necessary. The main thing we're probably missing out on is that it was possible to navigate through rows by using these keys:

  • Up arrow: Move 1 row up
  • Down arrow: Move 1 row down
  • Page up: Move 1 "page" up
  • Page down: Move 1 "page" down
  • Home: Move to the first row
  • End: Move to the last row

I don't think it's particularly important to add those now, but it would be nice if we could add those back. We're not using most of the other features of the FAST foundation data grid, so I think this covers the rest of what we are using pretty well.

Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/model-editor/HiddenMethodsRow.tsx
Comment thread extensions/ql-vscode/src/view/model-editor/ModeledMethodDataGrid.tsx Outdated
@robertbrignull
Copy link
Copy Markdown
Contributor Author

@koesie10, I think I've implemented everything from your review now. I've also done a bit more manual testing and as far as I can tell everything about the grid is working.

I ran into some extra problems with DataGridRow because it takes extra properties to be passed on to its children, such as ref and data-testid. I think I've solved everything but I'm not 100% happy with how understandable or maintainable it'll be going forward because you'll have to manually forward any future props as well. Let me know if you know of any better solutions.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing those changes. I have some additional comments, but the code looks really good already, these are mostly stylistic changes.

Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/common/DataGrid.tsx Outdated
Comment thread extensions/ql-vscode/src/view/model-editor/HiddenMethodsRow.tsx Outdated
@robertbrignull
Copy link
Copy Markdown
Contributor Author

Thanks for another review. The PR is all updated again.

@robertbrignull robertbrignull merged commit 947f495 into main Oct 19, 2023
@robertbrignull robertbrignull deleted the robertbrignull/model-table-alignment branch October 19, 2023 08:57
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.

2 participants