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

fix(frontend): form and content update when script is emptied #4887

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

HugoCasa
Copy link
Contributor

@HugoCasa HugoCasa commented Dec 10, 2024

Important

Simplify change dispatch logic in Editor.svelte by removing listenEmptyChanges property, ensuring changes are always dispatched, even when script is empty.

  • Behavior:
    • Remove listenEmptyChanges property from Editor.svelte.
    • Simplify change dispatch logic in Editor.svelte to always dispatch changes, even when script is empty.
  • Components:
    • Remove listenEmptyChanges from WorkerGroup.svelte when using Editor component.

This description was created by Ellipsis for 9448fe5. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 4cc902e in 14 seconds

More details
  • Looked at 38 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/lib/components/WorkerGroup.svelte:824
  • Draft comment:
    The listenEmptyChanges prop has been removed from the Editor component in Editor.svelte. Ensure that this change is intentional and that the component behaves as expected without this prop.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_PJ1fq9FlyR5b5Pw5


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@HugoCasa
Copy link
Contributor Author

@rubenfiszel the PR that added the check was https://github.com/windmill-labs/windmill/pull/1636/files#diff-6c66cfa8f98870ad45bef9f1a81a9e5e745c2a044eb9406cb47a97304ce2e45eR600
I think it was related to the template loading change in ScriptBuilder but i don't see why and i can't find any issues after the revert.

Copy link

cloudflare-workers-and-pages bot commented Dec 10, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9448fe5
Status: ✅  Deploy successful!
Preview URL: https://c15f4727.windmill.pages.dev
Branch Preview URL: https://hc-fix-detect-empty-script-e.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 9448fe5 in 15 seconds

More details
  • Looked at 40 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/lib/components/ScriptBuilder.svelte:224
  • Draft comment:
    The condition editor?.getScriptLang() == language ensures that the code is only set if the language matches, preventing unnecessary updates. This is a good improvement.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The condition in the if statement is checking both the script content and the language. This ensures that the code is only set if the content is different and the language matches. This is a logical improvement to prevent unnecessary updates.

Workflow ID: wflow_Me0NlK2HLn7sRYv8


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel merged commit 4ce4fba into main Dec 11, 2024
3 checks passed
@rubenfiszel rubenfiszel deleted the hc/fix-detect-empty-script-editor branch December 11, 2024 23:31
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants