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

[4/4] Add asset selection input for new syntax #26000

Merged
merged 17 commits into from
Nov 20, 2024

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Nov 19, 2024

Summary & Motivation

Adds an asset selection input that supports the new syntax and has auto-complete suggestions.

For now this is behind a hidden feature flag.

error state:
Screenshot 2024-11-19 at 1 49 11 AM

key/value suggestions:
Screenshot 2024-11-19 at 1 48 54 AM

base suggestions:
Screenshot 2024-11-19 at 1 48 47 AM

How I Tested These Changes

Manual testing for now until i figure out a good way to test CodeMirror...

To enable the feature flag:

localStorage.setItem("DAGSTER_FLAGS", JSON.stringify({
    ...JSON.parse(localStorage.DAGSTER_FLAGS),
    'flagAssetSelectionSyntax': true
}))
Screen.Recording.2024-11-19.at.3.02.01.AM.mov

@salazarm salazarm changed the base branch from master to salazarm/asset-selection-filtering November 19, 2024 06:46
Copy link

github-actions bot commented Nov 19, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-no79xzob1-elementl.vercel.app
https://salazarm-asset-selection-syntax-input.core-storybook.dagster-docs.io

Built with commit 51dbb2c.
This pull request is being automatically deployed with vercel-action

Comment on lines -28 to -46

/**
* Ignore hard navigation error (only happens in dev mode).
*/
if (process.env.NODE_ENV === 'development' && typeof window !== 'undefined') {
const originalError = window.Error;
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
window.Error = function Error(...args) {
if (args[0]?.includes('Invariant: attempted to hard navigate to the same URL')) {
return;
}
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const err = originalError(...args);
Object.setPrototypeOf(err, window.Error.prototype);
return err;
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need this anymore and it was changing error behavior in dev.

Copy link
Member

Choose a reason for hiding this comment

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

Worth making this its own PR since it's otherwise unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's related in that this ends up changing the prototype of the error which breaks an instanceof check in Antlr which causes my code not to work

Copy link

github-actions bot commented Nov 19, 2024

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-mgs8aqfgc-elementl.vercel.app
https://salazarm-asset-selection-syntax-input.components-storybook.dagster-docs.io

Built with commit 615f946.
This pull request is being automatically deployed with vercel-action

Comment on lines -28 to -46

/**
* Ignore hard navigation error (only happens in dev mode).
*/
if (process.env.NODE_ENV === 'development' && typeof window !== 'undefined') {
const originalError = window.Error;
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
window.Error = function Error(...args) {
if (args[0]?.includes('Invariant: attempted to hard navigate to the same URL')) {
return;
}
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const err = originalError(...args);
Object.setPrototypeOf(err, window.Error.prototype);
return err;
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Worth making this its own PR since it's otherwise unrelated?

@@ -194,7 +194,7 @@ export const NewConfigEditor = forwardRef<ConfigEditorHandle, ConfigEditorProps>
<StyledRawCodeMirror
value={configCode}
theme={['config-editor']}
options={options}
options={options as any}
Copy link
Member

Choose a reason for hiding this comment

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

Is this avoidable?

Copy link
Contributor Author

@salazarm salazarm Nov 20, 2024

Choose a reason for hiding this comment

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

I'll remove it, don't need it now that I changed the patched lint thing in the other PR. But no it's not because of the patched lint thing. It's expecting options that are not part of the official lint addon and so the types that the addon has cause an error.

@salazarm salazarm changed the title Add asset selection input for new syntax [4/4] Add asset selection input for new syntax Nov 20, 2024
Base automatically changed from salazarm/asset-selection-filtering to master November 20, 2024 18:32
@salazarm salazarm merged commit bc7acaa into master Nov 20, 2024
1 of 2 checks passed
@salazarm salazarm deleted the salazarm/asset-selection-syntax-input branch November 20, 2024 18:46
pskinnerthyme pushed a commit to pskinnerthyme/dagster that referenced this pull request Dec 16, 2024
## Summary & Motivation

Adds an asset selection input that supports the new syntax and has
auto-complete suggestions.

For now this is behind a hidden feature flag.

error state:
<img width="990" alt="Screenshot 2024-11-19 at 1 49 11 AM"
src="https://github.com/user-attachments/assets/e31986bd-f914-463f-a2d7-af590c86cbe5">

key/value suggestions:
<img width="736" alt="Screenshot 2024-11-19 at 1 48 54 AM"
src="https://github.com/user-attachments/assets/fb9cdd96-c4fd-4797-9e6c-2bcbc4d11984">

base suggestions:
<img width="490" alt="Screenshot 2024-11-19 at 1 48 47 AM"
src="https://github.com/user-attachments/assets/452a666a-fefe-46c8-8914-4db3aa483aaf">




## How I Tested These Changes

Manual testing for now until i figure out a good way to test
CodeMirror...

To enable the feature flag:
```
localStorage.setItem("DAGSTER_FLAGS", JSON.stringify({
    ...JSON.parse(localStorage.DAGSTER_FLAGS),
    'flagAssetSelectionSyntax': true
}))
```


https://github.com/user-attachments/assets/d158f5e0-527c-4350-ac79-7c7518a11c60
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