Skip to content

Update data extensions modelling table to new designs#2582

Merged
robertbrignull merged 8 commits intomainfrom
robertbrignull/data-table
Jul 6, 2023
Merged

Update data extensions modelling table to new designs#2582
robertbrignull merged 8 commits intomainfrom
robertbrignull/data-table

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Converts the data extensions table to match the new designs:
Screenshot 2023-07-04 at 17 22 35

The changes are:

  • Merge data into the "API or method" column, which now includes:
    • the package, class, method name, and parameter
    • a checkbox (which later on will show modelling status)
    • the usage count
    • an explicit link to view usages (though clicking the usage count number does still work)
  • The inputs in the table are disabled but still visible, instead of being completely hidden
    • The value in the cell is still hidden, otherwise I think it's odd seeing content in the disabled inputs
  • The red/green colouring is removed
    • It's my understanding this isn't mean to be a part of the new design. If that's not the case I can easily add it back in.

The bit I'm most unsure of is whether the whole "API or method" bit should be all one column or as separate columns. It depends on if we want the items to bunch together (like in the screenshot above), or if we want similar items to all align between rows. It's a little hard to tell from the designs since all of the dummy text is the same so stuff lines up regardless. @asiermartinez, what are your thoughts on whether items should align horizontally more or if the above screenshot looks good?

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 a review from a team as a code owner July 4, 2023 16:26
@robertbrignull
Copy link
Copy Markdown
Contributor Author

robertbrignull commented Jul 5, 2023

I'm really struggling to get this to work at small screen widths. It ends up looking something like the following two screenshots. Some or all of the columns become misaligned and it's not pretty 😢

With a medium width screen so only one column is affected:

Screenshot 2023-07-05 at 17 36 05

With a slightly smaller screen more columns become affected:

Screenshot 2023-07-05 at 18 07 58

I think I understand roughly what's going on here, but sadly I don't know of a way to fix it.

How is column width determined?

The width of each column is controlled by the CSS grid-template-columns property. This allows us to specify either how large each column should be in absolute terms or how much of the overall space we want them to take. In our case it's currently set as 0.4fr 0.15fr 0.15fr 0.15fr 0.15fr where fr means "free room", so the first column is 40% of the space and the rest are 15% each.

This all works fine except when it tries to reduce the column size below the size of the column contents. In our case the "Argument[0]: String" text is too long to fit in the allocated space, and so it refuses to reduce the column width below this. This is all fine and well, but apparently the grid system then gets confused and reduces the overall width so it no longer covers 100% of the container. In the bad row in the screenshot the 1st column and the 2nd/4th/5th are still in their 40%/15% relationship, but they're widths are lower than they should be so they don't fill all the remaining space correctly.

Can we use some other fancy grid-template-columns options to fix this?

I can't say it wouldn't be possible to somehow have it always fit the full width, but it wouldn't allow us to always line the columns up. The reason for that is that each row of the table is actually its own grid system. I don't know why it's done this way.

From reading https://www.w3schools.com/css/css_grid.asp and other sources I believe the intended way to use display: grid is that you have one grid and put all the cells from all rows into that one grid container. You use grid-row and grid-column to tell each cell where it should be located.

However, @vscode/webview-ui-toolkit decided that VSCodeDataGrid would not define the grid and in fact each VSCodeDataGridRow would have the display: grid property. This means that CSS cannot guarantee that the columns from different rows line up. We also cannot use grid-template-column options like max-content because each row would calculate the value differently. If you use max-content it's just a big free-for-all mess and looks like:

Screenshot 2023-07-05 at 17 46 41

Can we overflow to limit the size of cell contents?

We can add a overflow: hidden to the cell. This actually works surprisingly well for making the dropdown fit the size it's meant to, and the extra content gets cut off with ellipses correctly.

Screenshot 2023-07-05 at 17 57 00

Unfortunately it breaks the "popup" part of the dropdown where you pick other options, because that is nested under the dropdown element but only appears outside of it using position: absolute. So applying overflow: hidden to the cell stops the popup from appearing outside of it. Sadly I can't find any ways around this so I think overflow is a no-go.

Can we limit the size of the dropdown some other way?

I did find a way to make the columns always line up, and it's to put a max-width on the "control" element of the dropdown. In that case it will cut off the content and use ellipses.

Screenshot 2023-07-05 at 17 57 23

There's the small problem that this also affects the position of the arrow on the right side of the dropdown, but we could maybe work around this with fancier CSS determining the width.

Unfortunately the larger problem is that this modification is only possible using the manual browser dev tools. When building the elements in React it's not possible to apply this styling. Styled components does allow applying styles to child elements, but unfortunately the vscode-dropdown uses a shadow root which blocks global styles from applying to it. And sadly the dropdown component is not built to allow the application of styling to that element.

So what can we do?

  1. Maybe just live with it, and understand that the styling will go weird. Unfortunately it could happen on any screen size because it's about the interplay of screen width and dropdown content, and the content is determined from the types and other data from the CodeQL database. So for any screen there could still be dropdown contents that'll break it. But maybe that's acceptable.

  2. Another option is we stop using VSCodeDataGrid and VSCodeDropdown and go to more native HTML elements like table and select. We can apply styling to make them look like the VS Code versions. It's just more work if we want something that looks the same but behaves exactly as we want it to.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

Another option...

What if we don't specify grid-template-columns at all?

This is what the grid does currently, before the changes in this PR. With this (lack of) configuration, all the columns are given equal width. This actually leads to all of the columns being equally positioned between the various rows, and when the screen gets small they continue to shrink. When the screen gets really small it refuses to shrink the columns more and adds scrollbars, which is unusual but not terrible.

Screenshot 2023-07-06 at 09 59 39

The downside of this mode is that we can't control the size of the "API or method" column, so it ends up being too small for the content it is meant to contain. It doesn't look like the designs on wide screens, and it gets split onto multiple lines quite quickly as the screen width decreases.

Although it does have some good features, I don't think we want to go with this approach either 😢

@robertbrignull robertbrignull requested a review from a team as a code owner July 6, 2023 10:22
@robertbrignull
Copy link
Copy Markdown
Contributor Author

I've tried making my own dropdown component, copying the styling from VSCodeDropdown, and it works quite well. Here are some screenshots with a wide and a narrow window:

Screenshot 2023-07-06 at 11 02 51 Screenshot 2023-07-06 at 11 03 05

I think the only downside of this approach is that we're making our own custom components instead of using the VS Code ones. But in this case I'm not sure we have a choice if we want it to behave the way we want it to.

@robertbrignull robertbrignull force-pushed the robertbrignull/data-table branch from 2fe4dd0 to b6a685b Compare July 6, 2023 10:44
@robertbrignull robertbrignull force-pushed the robertbrignull/data-table branch from b6a685b to 903e8c6 Compare July 6, 2023 10:48
`;

type Props = {
value: string | undefined | false;
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.

Including | false here is kinda nice because it means in MethodRow.tsx we can pass in showInputCell && modeledMethod?.input instead of having to do showInputCell ?modeledMethod?.input : undefined. But this isn't essential and I'm happy to change the type to just be string | undefined if that's preferable.

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.

Although, I just realised we can do this logic of hiding the value when the input is disabled in the Dropdown component. So ignore the previous comment. The type can be simpler and we can just ignore the value when the input is disabled.

Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Styling looks good! Thanks for digging in to the issues with the VS Code data grid.

(As mentioned offline, we can open an issue with the VS Code UI toolkit and potentially switch back to that if they fix the styling.)

@robertbrignull robertbrignull merged commit 261f11e into main Jul 6, 2023
@robertbrignull robertbrignull deleted the robertbrignull/data-table branch July 6, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants