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

Add an appStatus to app controllers and scope #7984

Merged
merged 21 commits into from
Aug 19, 2016

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Aug 12, 2016

This PR adds an appStatus to the controller and scopes for Discover, Visualize and Dashboard. It uses this status to track whether or not the app is "dirty", that is, if it has been modified since it was last saved.

  • Add state monitoring module
  • Refactor Discover and Visualize into directives
  • Add dirty checking to apps based on appState, and in the case of Visualize, editableVis as well
  • Fix config rendering, firing postLink after injecting in the DOM so that directives with a require will function

w33ble added 5 commits August 11, 2016 17:11
without this change, the directive is rendered in isolation and can not properly read the DOM, so require statements in custom config controls don't work
continue to use all the same controller code
cleanupFn(unlisten);
}

module.exports = monitorStateChanges;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use es6 modules: export default monitorStateChanges;

@epixa
Copy link
Contributor

epixa commented Aug 12, 2016

Can you get some tests in for these changes wherever it is practical?

<i class="fa fa-chevron-up"></i>
</div>
</div>
`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cleanup job!

@cjcenizal
Copy link
Contributor

cjcenizal commented Aug 15, 2016

I left a few comments, but I also have a suggestion for making the interface for this new service more intuitive.

How about if it was used this way:

const StateChangeMonitor = require('ui/state_management/state_change_monitor');

const stateChangeMonitor = new StateChangeMonitor($state, stateDefaults);

stateChangeMonitor.onStateChange(isDirty => {
  $appStatus.isDirty = isDirty;
});

$scope.$on('$destroy', () => {
  stateChangeMonitor.destroy();
});

I find this easier to follow for a few reasons:

  1. There's no possibility of having shared state between controllers, because each owns its own instance of stateChangeMonitor.
  2. Passing the constructor references to a default state and a changing state implies that it is primarily concerned about those two things, which implies its job is to do comparisons.
  3. The event-handler-registration method is explicit, and supports the idea that its job is to do comparisons.
  4. Calling destroy is also very explicit.

The only thing I think I'm not anticipating is the use case where you force the dirtyChecker to return true if it's already dirty. If you like my idea then maybe we can discuss the requirement in that case and figure out how to work it in?

w33ble added 5 commits August 15, 2016 15:38
fix passing by reference, trigger dirty on column changes
fix objects passed by reference there as well
a refactor of how monitor_state_changes works
@tylersmalley
Copy link
Contributor

This looks great! Reviewed the code and implemented a test in Editor.

LGTM

@tylersmalley tylersmalley removed their assignment Aug 18, 2016
@cjcenizal
Copy link
Contributor

LGTM once w33ble#7 gets merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants