Skip to content

Use one grid cell per model input, and have the method name span multiple rows #2995

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.

Sign up for GitHub

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

Merged
robertbrignull merged 2 commits into from
Oct 19, 2023
Merged

Use one grid cell per model input, and have the method name span multiple rows #2995

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09a8d29
Use one grid cell per input
robertbrignull Oct 18, 2023
50f77e7
Use Fragment to add key to map output
robertbrignull Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 28 additions & 42 deletions extensions/ql-vscode/src/view/model-editor/MethodRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,14 @@ import {
VSCodeProgressRing,
} from "@vscode/webview-ui-toolkit/react";
import * as React from "react";
import { forwardRef, useCallback, useEffect, useMemo, useRef } from "react";
import {
Fragment,
forwardRef,
useCallback,
useEffect,
useMemo,
useRef,
} from "react";
import { styled } from "styled-components";
import { vscode } from "../vscode-api";

Expand All @@ -25,12 +32,6 @@ import { Codicon } from "../common";
import { canAddNewModeledMethod } from "../../model-editor/shared/multiple-modeled-methods";
import { DataGridCell, DataGridRow } from "../common/DataGrid";

const MultiModelColumn = styled(DataGridCell)`
display: flex;
flex-direction: column;
gap: 0.5em;
`;

const ApiOrMethodRow = styled.div`
min-height: calc(var(--input-height) * 1px);
display: flex;
Expand Down Expand Up @@ -162,7 +163,7 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
ref={ref}
focused={revealedMethodSignature === method.signature}
>
<DataGridCell>
<DataGridCell gridRow={`span ${modeledMethods.length}`}>
<ApiOrMethodRow>
<ModelingStatusIndicator status={modelingStatus} />
<MethodClassifications method={method} />
Expand Down Expand Up @@ -199,54 +200,41 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
)}
</>
)}
{!props.modelingInProgress && (
<>
<MultiModelColumn>
{modeledMethods.map((modeledMethod, index) => (
{!props.modelingInProgress &&
modeledMethods.map((modeledMethod, index) => (
<Fragment key={index}>
<DataGridCell>
<ModelTypeDropdown
key={index}
Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Oct 18, 2023

Choose a reason for hiding this comment

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

I was a little concerned about removing these keys. I've done manual tested and I haven't seen anything not updating correctly. Also, the default behaviour if you don't specify a key is to use the index.

Copy link
Copy Markdown
Member

@koesie10 koesie10 Oct 19, 2023

Choose a reason for hiding this comment

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

I think the key can be removed here since this is not a top-level element in the map anymore. However, I think we should still add a key on the <> that is now the top-level literal. The only reason ESLint doesn't give an error is that it doesn't seem to detect it on <>, but changing it to its concrete implementation (using <Fragment> instead of <>) does give the error.

If we don't add the key there, we will get an error in the next major version of eslint-react: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-key.md#checkfragmentshorthand-default-false. It's probably best to add it by changing it to <Fragment key={index}>.

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Oct 19, 2023

Choose a reason for hiding this comment

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

Ah nice. I did try adding a key but it won't even allow it on a <>. Good suggestion to switch to <Fragment> instead.

method={method}
modeledMethod={modeledMethod}
onChange={modeledMethodChangedHandlers[index]}
/>
))}
</MultiModelColumn>
<MultiModelColumn>
{modeledMethods.map((modeledMethod, index) => (
</DataGridCell>
<DataGridCell>
<ModelInputDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={modeledMethodChangedHandlers[index]}
/>
))}
</MultiModelColumn>
<MultiModelColumn>
{modeledMethods.map((modeledMethod, index) => (
</DataGridCell>
<DataGridCell>
<ModelOutputDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={modeledMethodChangedHandlers[index]}
/>
))}
</MultiModelColumn>
<MultiModelColumn>
{modeledMethods.map((modeledMethod, index) => (
</DataGridCell>
<DataGridCell>
<ModelKindDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={modeledMethodChangedHandlers[index]}
/>
))}
</MultiModelColumn>
{viewState.showMultipleModels && (
<MultiModelColumn>
{modeledMethods.map((_, index) =>
index === modeledMethods.length - 1 ? (
</DataGridCell>
{viewState.showMultipleModels && (
<DataGridCell>
{index === modeledMethods.length - 1 ? (
<CodiconRow
key={index}
appearance="icon"
aria-label="Add new model"
onClick={handleAddModelClick}
Expand All @@ -256,19 +244,17 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
</CodiconRow>
) : (
<CodiconRow
key={index}
appearance="icon"
aria-label="Remove model"
onClick={removeModelClickedHandlers[index]}
>
<Codicon name="trash" />
</CodiconRow>
),
)}
</MultiModelColumn>
)}
</>
)}
)}
</DataGridCell>
)}
</Fragment>
))}
</DataGridRow>
);
},
Expand Down