-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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(editor): Never show Pinned Data Callout for Input Panel #12446
Conversation
@@ -1277,7 +1277,12 @@ defineExpose({ enterEditMode }); | |||
<template> | |||
<div :class="['run-data', $style.container]" @mouseover="activatePane"> | |||
<N8nCallout | |||
v-if="pinnedData.hasData.value && !editMode.enabled && !isProductionExecutionPreview" | |||
v-if=" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the quick fix. Can you please add a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't really a bug, but rather a change in (semi-)intended behavior.
I can add tests for the callout banner in general (we don't have any, only for the pin button), but it's probably best logistically to separate that into a tech debt ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luckily we already have good coverage for RunData. Can we simply add a unit test for this there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out the input panel has a snapshot test, I have pushed a change to update that. It will catch this showing up accidentally. (This also means the change was more or less intentional on our end back then, since we explicitly updated the snapshot, so just some miscommunication.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a clear check if there's no pinned data callout in panel, to avoid this bug being reintroduced. Because snapshots don't convey those requirements and get updated often without full consideration/review.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the feedback and adding the tests.
✅ All Cypress E2E specs passed |
n8n Run #8592
Run Properties:
|
Project |
n8n
|
Branch Review |
ADO-3061
|
Run status |
Passed #8592
|
Run duration | 04m 47s |
Commit |
cffab03f90: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 CharlieKolb 🗃️ e2e/*
|
Committer | Charlie Kolb |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
0
|
Skipped |
0
|
Passing |
484
|
View all changes introduced in this branch ↗︎ |
Got released with |
1 similar comment
Got released with |
Got released with |
1 similar comment
Got released with |
Summary
Always hide the banner if we're in the input panel.
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-3061/pinned-data-notice-appearing-in-input-pane
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)