-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Carry over filter extra fields with in incomplete state #102509
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
if (referencedOperation.shiftable) { | ||
column.timeShift = (previousColumn as ReferenceBasedIndexPatternColumn).timeShift; | ||
} | ||
return column; |
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 function is returning the column but also mutating it. We had issues with this kind of functions before - could we make this immutable so a new column with filter/timeshift is returned?
@@ -30,6 +32,11 @@ import { generateId } from '../../id_generator'; | |||
import { ReferenceBasedIndexPatternColumn } from './definitions/column_types'; | |||
import { FormulaIndexPatternColumn, regenerateLayerFromAst } from './definitions/formula'; | |||
|
|||
interface ColumnFilters { |
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.
The whole PR is using "filters" as a name of the extra params passed around like this. While they are just filters for now, it's more about additional params - could we rename this to incompleteParams
, ColumnParams
and so on?
const incompleteColumn: { | ||
operationType: OperationType; | ||
filter?: unknown; | ||
timeShift?: unknown; |
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.
timeScale should probably be treated in the same way (another reason for not calling it filters0
@elasticmachine merge upstream |
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.
LGTM once one nit is addressed
@@ -141,6 +151,24 @@ export function insertOrReplaceColumn(args: ColumnChange): IndexPatternLayer { | |||
return insertNewColumn(args); | |||
} | |||
|
|||
function ensureCompatibleFiltersAreMoved<T extends ColumnAdvancedParams>( |
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.
nit: ensureCompatibleParamsAreMoved
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…ic#102509) * 🐛 Carry over filter extra fields with in incomplete state * 👌 Integrated feedback * 👌 Integrated feedback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…) (#102899) * 🐛 Carry over filter extra fields with in incomplete state * 👌 Integrated feedback * 👌 Integrated feedback Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Summary
Fix #102202
This PR saves in the incomplete state the operation filters, and when the current operation conflicts get resolved, moves them within the newly created column.
The actual transition is a two step phase:
incompleteColumn
is created and stored as the field type is conflicting with the new operationinsertNewColumn
here to know about the previous filters paramsChecklist
Delete any items that are not applicable to this PR.
For maintainers