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

[Lens] disable auto apply toggle flag strategy #125157

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Feb 9, 2022

Summary

Counterpart to #125158.

In this approach, we mark whether changes should be applied by way of a boolean flag in the state.

In some ways, this is simple, but when we want the app to start with auto-apply turned off, the workspace panel has to ignore the setting until the initial render is finished which is a little tricky. Applying changes is also done by way of toggling the setting on and off. This sometimes makes the switch flash a bit.

@drewdaemon drewdaemon force-pushed the 74490/disable-auto-apply-toggle-flag-strategy branch from 72d3fbd to 8e15e08 Compare February 9, 2022 21:29
Comment on lines +310 to +311
dispatch(enableApplyChanges());
setImmediate(() => dispatch(disableApplyChanges())); // wait for render
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a little hacky?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this may incur into race conditions. But the point is still valid, having a nextFrame function would be really useful here and also elsewhere

@@ -28,6 +28,7 @@ export interface PreviewState {
export interface EditorFrameState extends PreviewState {
activeDatasourceId: string | null;
stagedPreview?: PreviewState;
applyChangesDisabled?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the boolean flag we add to the state.

@mbondyra
Copy link
Contributor

mbondyra commented Feb 10, 2022

Do I understand this correctly that in this approach we wouldn't be able to have a potential discard button because the last applied state is not stored anywhere? Not sure if we're planning on having it, just thinking about it based on Visualize design.

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Feb 10, 2022

Thanks @dej611 and @mbondyra. Great points both here and in Slack.

I think it has become clear that the other approach is best because of the capabilities it affords

  • Easy to check if the user has applied their changes—allows us to provide a better UX
  • Discard button
  • Option to expand into more advanced functionality such as saving drafts, etc

It also feels less "tricky." I'll move forward with the other approach knowing we've chosen the right path.

@drewdaemon drewdaemon closed this Feb 10, 2022
@drewdaemon drewdaemon reopened this Feb 24, 2022
@kibana-ci
Copy link
Collaborator

kibana-ci commented Feb 24, 2022

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #1 / Initializing the store should initialize all datasources with state from doc
  • [job] [logs] Default CI Group #3 / lens app lens smokescreen tests should handle edge cases in reference-based operations
  • [job] [logs] Default CI Group #3 / lens app lens smokescreen tests should handle edge cases in reference-based operations
  • [job] [logs] Default CI Group #4 / lens app time shift should show an error if terms is used and provide a fix action
  • [job] [logs] Default CI Group #4 / lens app time shift should show an error if terms is used and provide a fix action
  • [job] [logs] Jest Tests #1 / workspace_panel should show an error message if the expression fails to parse
  • [job] [logs] Jest Tests #1 / workspace_panel should show an error message if there are missing indexpatterns in the visualization
  • [job] [logs] Jest Tests #1 / workspace_panel should show an error message if validation on both datasource and visualization do not pass
  • [job] [logs] Jest Tests #1 / workspace_panel should show an error message if validation on datasource does not pass
  • [job] [logs] Jest Tests #1 / workspace_panel should show an error message if validation on visualization does not pass

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 748 750 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.1MB 1.1MB +4.9KB
Unknown metric groups

ESLint disabled in files

id before after diff
apm 15 14 -1
securitySolution 66 65 -1
uptime 7 6 -1
total -3

ESLint disabled line counts

id before after diff
apm 85 82 -3
uptime 48 42 -6
total -9

References to deprecated APIs

id before after diff
canvas 70 64 -6
fleet 5 4 -1
lens 237 185 -52
monitoring 40 28 -12
stackAlerts 183 157 -26
upgradeAssistant 8 3 -5
total -102

Total ESLint disabled count

id before after diff
apm 100 96 -4
securitySolution 513 512 -1
uptime 55 48 -7
total -12

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@drewdaemon drewdaemon closed this Mar 22, 2022
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.

4 participants