[ES|QL] Disable the Save button any time the ES|QL query is changed#267642
Conversation
bartoval
added
bug
bartoval
changed the title
bartoval
changed the title
| }); | ||
| }, []); | ||
|
|
||
| const formIsInvalid = useMemo(() => { |
There was a problem hiding this comment.
No need useMemo here but all constants are wrapped into it in thi file.
| import { ChooseColumnPopover } from './choose_column_popover'; | ||
| import { ControlLabel, ControlSelectionType } from './shared_form_components'; | ||
|
|
||
| export type ValuesPreviewStatus = 'valid' | 'stale' | 'invalid'; |
There was a problem hiding this comment.
It can also simply be a boolean value, or e620cea ,
The latter provides a more comprehensive description of the state.
bartoval
force-pushed
the
esql-vlues-save-disabled
branch
from
fa13e4c to
e279c9c
Compare
|
Pinging @elastic/kibana-esql (Team:ESQL) |
stratoula
left a comment
stratoula
left a comment
There was a problem hiding this comment.
Works very nice, just some small comments from me
| import type { ISearchGeneric } from '@kbn/search-types'; | ||
| import type { monaco } from '@kbn/monaco'; | ||
| import { ValueControlForm } from './value_control_form'; | ||
| import type { monaco } from '@kbn/code-editor'; |
There was a problem hiding this comment.
If we are changing this to code-editor lets change all the occurences because now we have reference both in code editor and monaco
There was a problem hiding this comment.
Let's add at least one test here to test the new behavior
| const [variableType, setVariableType] = useState<ESQLVariableType>(initialVariableType); | ||
|
|
||
| const [formIsInvalid, setFormIsInvalid] = useState(false); | ||
| const [valueControlFormStatus, setValueControlFormStatus] = useState<ValueControlFormStatus>({ |
There was a problem hiding this comment.
This is a bit optimistic, sometimes we initialize in wrong queries (when the user tries to create the control from the query) so this won't be valid. till the validation runs and disables the button the user can click save. I would initialize to stale
|
|
||
| export type ValuesPreviewStatus = 'valid' | 'stale' | 'invalid'; | ||
|
|
||
| export interface ValueControlFormStatus { |
There was a problem hiding this comment.
Why are we overcomplicating the state here? We dont need the ValueControlFormStatus, just the valuesPreviewStatus: ValuesPreviewStatus;
| import { ChooseColumnPopover } from './choose_column_popover'; | ||
| import { ControlLabel, ControlSelectionType } from './shared_form_components'; | ||
|
|
||
| export type ValuesPreviewStatus = 'valid' | 'stale' | 'invalid'; |
There was a problem hiding this comment.
Isnt a boolean enough? IsValid is enough. We dont have any plans to have a different UI for stale and invalid
bartoval
force-pushed
the
esql-vlues-save-disabled
branch
2 times, most recently
from
da48d54 to
61aad7f
Compare
bartoval
force-pushed
the
esql-vlues-save-disabled
branch
from
61aad7f to
4d01b43
Compare
stratoula
left a comment
stratoula
left a comment
There was a problem hiding this comment.
I didnt test again but it looks good to me, thanks Val
💚 Build Succeeded
Metrics [docs]Async chunks
History
cc @bartoval |
3 participants
Summary
Closes #266720
disable.mp4
Fixes the ES|QL values control flyout so the Save button is disabled when the values query has been edited after the last successful run.
Now if the query text changes, it can no longer be saved until the query is run again