Skip to content

Commit

Permalink
address code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@stacey-gammon
stacey-gammon committed Mar 23, 2017
1 parent badf336 commit 0c61fc7
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 76 deletions.
4 changes: 2 additions & 2 deletions src/core_plugins/kibana/public/dashboard/dashboard.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@
<!-- Filters. -->
<filter-bar state="state"></filter-bar>

<div ng-show="showEditHelpText()" class="text-center start-screen">
<div ng-show="getShouldShowEditHelp()" class="text-center start-screen">
<h2>This dashboard is empty. Let's fill it up!</h2>
<p>Click the <a class="btn btn-xs navbtn-inverse" ng-click="kbnTopNav.open('add'); toggleAddVisualization = !toggleAddVisualization" aria-label="Add visualization">Add</a> button in the menu bar above to add a visualization to the dashboard. <br/>If you haven't setup a visualization yet visit <a href="#/visualize" title="Visualize">"Visualize"</a> to create your first visualization.</p>
</div>

<div ng-show="showViewHelpText()" class="text-center start-screen">
<div ng-show="getShouldShowViewHelp()" class="text-center start-screen">
<h2>This dashboard is empty. Let's fill it up!</h2>
<p>Click the <a class="btn btn-xs navbtn-inverse" ng-click="kbnTopNav.open('edit');" aria-label="Edit">Edit</a> button in the menu bar above to start working on your new dashboard.
</div>
Expand Down
16 changes: 9 additions & 7 deletions src/core_plugins/kibana/public/dashboard/dashboard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
dashboardState.getIsDirty(timefilter));
$scope.newDashboard = () => { kbnUrl.change(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {}); };
$scope.saveState = () => dashboardState.saveState();
$scope.showEditHelpText = () => !dashboardState.getPanels().length && dashboardState.getIsEditMode();
$scope.showViewHelpText = () => !dashboardState.getPanels().length && dashboardState.getIsViewMode();
$scope.getShouldShowEditHelp = () => !dashboardState.getPanels().length && dashboardState.getIsEditMode();
$scope.getShouldShowViewHelp = () => !dashboardState.getPanels().length && dashboardState.getIsViewMode();

$scope.toggleExpandPanel = (panelIndex) => {
if ($scope.expandedPanel && $scope.expandedPanel.panelIndex === panelIndex) {
Expand Down Expand Up @@ -179,8 +179,8 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,

const onChangeViewMode = (newMode) => {
const isPageRefresh = newMode === dashboardState.getViewMode();
const leavingEditMode = !isPageRefresh && newMode === DashboardViewMode.VIEW;
const willLoseChanges = leavingEditMode && dashboardState.getIsDirty(timefilter);
const isLeavingEditMode = !isPageRefresh && newMode === DashboardViewMode.VIEW;
const willLoseChanges = isLeavingEditMode && dashboardState.getIsDirty(timefilter);

if (!willLoseChanges) {
updateViewMode(newMode);
Expand All @@ -189,8 +189,10 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,

function revertChangesAndExitEditMode() {
dashboardState.resetState();
const refreshUrl = dashboardState.getReloadDashboardUrl();
kbnUrl.change(refreshUrl.url, refreshUrl.options);
const refreshUrl = dash.id ?
DashboardConstants.EXISTING_DASHBOARD_URL : DashboardConstants.CREATE_NEW_DASHBOARD_URL;
const refreshUrlOptions = dash.id ? { id: dash.id } : {};
kbnUrl.change(refreshUrl, refreshUrlOptions);
// This is only necessary for new dashboards, which will default to Edit mode.
updateViewMode(DashboardViewMode.VIEW);

Expand All @@ -203,7 +205,7 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
}

confirmModal(
DashboardStrings.getUnsavedChangesWarningMessage(dashboardState.getChangedFiltersForDisplay(timefilter)),
getUnsavedChangesWarningMessage(dashboardState.getChangedFilterTypes(timefilter)),
{
onConfirm: revertChangesAndExitEditMode,
onCancel: _.noop,
Expand Down
105 changes: 47 additions & 58 deletions src/core_plugins/kibana/public/dashboard/dashboard_state.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,19 @@ function areTimesEqual(timeA, timeB) {
export class DashboardState {
/**
*
* @param dashboard {SavedDashboard}
* @param savedDashboard {SavedDashboard}
* @param AppState {AppState}
*/
constructor(dashboard, AppState) {
this.dashboard = dashboard;
constructor(savedDashboard, AppState) {
this.savedDashboard = savedDashboard;

this.stateDefaults = getStateDefaults(this.dashboard);
this.stateDefaults = getStateDefaults(this.savedDashboard);

this.appState = new AppState(this.stateDefaults);
this.uiState = this.appState.makeStateful('uiState');
this.isDirty = false;

// We can't compare the filters stored on this.appState to this.dashboard because in order to apply
// We can't compare the filters stored on this.appState to this.savedDashboard because in order to apply
// the filters to the visualizations, we need to save it on the dashboard. We keep track of the original
// filter state in order to let the user know if their filters changed and provide this specific information
//in the 'lose changes' warning message.
Expand All @@ -88,8 +88,8 @@ export class DashboardState {
// The right way to fix this might be to ensure the defaults object stored on state is a deep
// clone, but given how much code uses the state object, I determined that to be too risky of a change for
// now. TODO: revisit this!
this.stateDefaults = getStateDefaults(this.dashboard);
// The original query won't be restored by the above because the query on this.dashboard is applied
this.stateDefaults = getStateDefaults(this.savedDashboard);
// The original query won't be restored by the above because the query on this.savedDashboard is applied
// in place in order for it to affect the visualizations.
this.stateDefaults.query = this.lastSavedDashboardFilters.query;
// Need to make a copy to ensure they are not overwritten.
Expand All @@ -102,13 +102,13 @@ export class DashboardState {
}

/**
* Returns an object which contains the current filter state of this.dashboard.
* Returns an object which contains the current filter state of this.savedDashboard.
* @returns {{timeTo: String, timeFrom: String, filterBars: Array, query: Object}}
*/
getFilterState() {
return {
timeTo: this.dashboard.timeTo,
timeFrom: this.dashboard.timeFrom,
timeTo: this.savedDashboard.timeTo,
timeFrom: this.savedDashboard.timeFrom,
filterBars: this.getDashboardFilterBars(),
query: this.getDashboardQuery()
};
Expand Down Expand Up @@ -153,15 +153,15 @@ export class DashboardState {
* @returns {boolean}
*/
getIsTimeSavedWithDashboard() {
return this.dashboard.timeRestore;
return this.savedDashboard.timeRestore;
}

getDashboardFilterBars() {
return FilterUtils.getFilterBarsForDashboard(this.dashboard);
return FilterUtils.getFilterBarsForDashboard(this.savedDashboard);
}

getDashboardQuery() {
return FilterUtils.getQueryFilterForDashboard(this.dashboard);
return FilterUtils.getQueryFilterForDashboard(this.savedDashboard);
}

getLastSavedFilterBars() {
Expand Down Expand Up @@ -260,56 +260,56 @@ export class DashboardState {
}

/**
* @return {boolean} True if filters (query, filter bar filters, and time picker if time is stored
* with the dashboard) have changed since the last saved state (or if the dashboard hasn't been saved,
* the default state).
* @param timeFilter
* @returns {Array.<string>} An array of user friendly strings indicating the filter types that have changed.
*/
getFiltersChanged(timeFilter) {
return (
this.getQueryChanged() ||
this.getFilterBarChanged() ||
(this.dashboard.timeRestore && this.getTimeChanged(timeFilter))
);
}

getChangedFiltersForDisplay(timeFilter) {
getChangedFilterTypes(timeFilter) {
const changedFilters = [];
if (this.getFilterBarChanged()) {
changedFilters.push('filter');
}
if (this.getQueryChanged()) {
changedFilters.push('query');
}
if (this.dashboard.timeRestore && this.getTimeChanged(timeFilter)) {
if (this.savedDashboard.timeRestore && this.getTimeChanged(timeFilter)) {
changedFilters.push('time range');
}
return changedFilters;
}

/**
* @return {boolean} True if filters (query, filter bar filters, and time picker if time is stored
* with the dashboard) have changed since the last saved state (or if the dashboard hasn't been saved,
* the default state).
*/
getFiltersChanged(timeFilter) {
return this.getChangedFilterTypes(timeFilter).length > 0;
}

/**
* Updates timeFilter to match the time saved with the dashboard.
* @param timeFilter
* @param quickTimeRanges
*/
syncTimefilterWithDashboard(timeFilter, quickTimeRanges) {
if (!this.getIsTimeSavedWithDashboard()) {
throw 'The time is not saved with this dashboard so should not be synced.';
throw new Error('The time is not saved with this dashboard so should not be synced.');
}

timeFilter.time.to = this.dashboard.timeTo;
timeFilter.time.from = this.dashboard.timeFrom;
const isMoment = moment(this.dashboard.timeTo).isValid();
timeFilter.time.to = this.savedDashboard.timeTo;
timeFilter.time.from = this.savedDashboard.timeFrom;
const isMoment = moment(this.savedDashboard.timeTo).isValid();
if (isMoment) {
timeFilter.time.mode = 'absolute';
} else {
const quickTime = _.find(
quickTimeRanges,
(timeRange) => timeRange.from === this.dashboard.timeFrom && timeRange.to === this.dashboard.timeTo);
(timeRange) => timeRange.from === this.savedDashboard.timeFrom && timeRange.to === this.savedDashboard.timeTo);

timeFilter.time.mode = quickTime ? 'quick' : 'relative';
}
if (this.dashboard.refreshInterval) {
timeFilter.refreshInterval = this.dashboard.refreshInterval;
if (this.savedDashboard.refreshInterval) {
timeFilter.refreshInterval = this.savedDashboard.refreshInterval;
}
}

Expand All @@ -333,19 +333,19 @@ export class DashboardState {
this.saveState();

const timeRestoreObj = _.pick(timeFilter.refreshInterval, ['display', 'pause', 'section', 'value']);
this.dashboard.title = this.appState.title;
this.dashboard.timeRestore = this.appState.timeRestore;
this.dashboard.panelsJSON = toJson(this.appState.panels);
this.dashboard.uiStateJSON = toJson(this.uiState.getChanges());
this.dashboard.timeFrom = this.dashboard.timeRestore ? convertTimeToString(timeFilter.time.from) : undefined;
this.dashboard.timeTo = this.dashboard.timeRestore ? convertTimeToString(timeFilter.time.to) : undefined;
this.dashboard.refreshInterval = this.dashboard.timeRestore ? timeRestoreObj : undefined;
this.dashboard.optionsJSON = toJson(this.appState.options);

return this.dashboard.save()
this.savedDashboard.title = this.appState.title;
this.savedDashboard.timeRestore = this.appState.timeRestore;
this.savedDashboard.panelsJSON = toJson(this.appState.panels);
this.savedDashboard.uiStateJSON = toJson(this.uiState.getChanges());
this.savedDashboard.timeFrom = this.savedDashboard.timeRestore ? convertTimeToString(timeFilter.time.from) : undefined;
this.savedDashboard.timeTo = this.savedDashboard.timeRestore ? convertTimeToString(timeFilter.time.to) : undefined;
this.savedDashboard.refreshInterval = this.savedDashboard.timeRestore ? timeRestoreObj : undefined;
this.savedDashboard.optionsJSON = toJson(this.appState.options);

return this.savedDashboard.save()
.then((id) => {
this.lastSavedDashboardFilters = this.getFilterState();
this.stateDefaults = getStateDefaults(this.dashboard);
this.stateDefaults = getStateDefaults(this.savedDashboard);
this.stateDefaults.viewMode = DashboardViewMode.VIEW;
// Make sure new app state defaults are using the new defaults.
this.appState.setDefaults(this.stateDefaults);
Expand All @@ -361,11 +361,11 @@ export class DashboardState {
applyFilters(query, filters) {
this.appState.query = query;
if (this.appState.query) {
this.dashboard.searchSource.set('filter', _.union(filters, [{
this.savedDashboard.searchSource.set('filter', _.union(filters, [{
query: this.appState.query
}]));
} else {
this.dashboard.searchSource.set('filter', filters);
this.savedDashboard.searchSource.set('filter', filters);
}

this.saveState();
Expand Down Expand Up @@ -401,17 +401,6 @@ export class DashboardState {
if (this.stateMonitor) {
this.stateMonitor.destroy();
}
this.dashboard.destroy();
}

/**
* Returns a url and url options that can be used to reload the page.
* @returns {{url: string, options: Object}}
*/
getReloadDashboardUrl() {
const url = this.dashboard.id ?
DashboardConstants.EXISTING_DASHBOARD_URL : DashboardConstants.CREATE_NEW_DASHBOARD_URL;
const options = this.dashboard.id ? { id: this.dashboard.id } : {};
return { url, options };
this.savedDashboard.destroy();
}
}
9 changes: 6 additions & 3 deletions src/core_plugins/kibana/public/dashboard/panel/panel.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
<div class="btn-group">
<a aria-label="Expand"
data-test-subj="dashboardPanelExpandIcon"
ng-click="toggleExpand()">
ng-click="toggleExpand()"
>
<span class="fa" ng-class="{'fa-expand': !isExpanded, 'fa-compress': isExpanded}"></span>
</a>
<a aria-label="Edit"
Expand All @@ -19,13 +20,15 @@
<a aria-label="Move"
data-test-subj="dashboardPanelMoveIcon"
ng-show="!isViewOnlyMode() && !isExpanded"
class="panel-move">
class="panel-move"
>
<i aria-hidden="true" class="fa fa-arrows"></i>
</a>
<a aria-label="Remove"
data-test-subj="dashboardPanelRemoveIcon"
ng-show="!isViewOnlyMode() && !isExpanded"
ng-click="remove()">
ng-click="remove()"
>
<i aria-hidden="true" class="fa fa-times"></i>
</a>
</div>
Expand Down
16 changes: 10 additions & 6 deletions test/functional/apps/dashboard/_dashboard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,17 @@ bdd.describe('dashboard tab', function describeIndexTests() {
});
});

bdd.it('filters when a pie chart slice is clicked', async function () {
let descriptions = await PageObjects.dashboard.getFilterDescriptions(1000);
expect(descriptions.length).to.equal(0);
bdd.describe('filters', async function () {
bdd.it('are not selected by default', async function () {
const descriptions = await PageObjects.dashboard.getFilterDescriptions(1000);
expect(descriptions.length).to.equal(0);
});

await PageObjects.dashboard.filterOnPieSlice();
descriptions = await PageObjects.dashboard.getFilterDescriptions();
expect(descriptions.length).to.equal(1);
bdd.it('are added when a pie chart slice is clicked', async function () {
await PageObjects.dashboard.filterOnPieSlice();
const descriptions = await PageObjects.dashboard.getFilterDescriptions();
expect(descriptions.length).to.equal(1);
});
});

bdd.it('retains dark theme in state', async function () {
Expand Down

0 comments on commit 0c61fc7

Please sign in to comment.