Skip to content

Commit

Permalink
Panel buttons should be uniform: change all buttons to "Apply"; all b…
Browse files Browse the repository at this point in the history
…uttons should be at bottom (#1524)

* update feature list apply button

* update axis config dialog apply button

* fix lint

* update apply button for model overview

* fix lint

* address comment and update localization for shiftCohort

* fix lint

* fix e2e

* fix e2e failures
  • Loading branch information
tongyu-microsoft authored Jun 29, 2022
1 parent 8be72b5 commit 94dd072
Showing 18 changed files with 67 additions and 62 deletions.
Original file line number Diff line number Diff line change
@@ -42,7 +42,7 @@ export function describeAxisConfigDialog(
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.contains("Apply")
.click();
cy.get(
'#DatasetExplorerChart div[class*="rotatedVerticalBox"] button:eq(0)'
@@ -58,7 +58,7 @@ export function describeAxisConfigDialog(
.click()
.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.contains("Apply")
.click();
});
}
@@ -97,7 +97,7 @@ export function describeAxisConfigDialog(
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.contains("Apply")
.click();
cy.get(
'#DatasetExplorerChart div[class*="horizontalAxis"] button:eq(0)'
@@ -111,7 +111,7 @@ export function describeAxisConfigDialog(
.click()
.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.contains("Apply")
.click();
});
}
Original file line number Diff line number Diff line change
@@ -23,10 +23,7 @@ export function describeAxisConfigDialog(): void {
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.click();
cy.get("#AxisConfigPanel").find("button").contains("Apply").click();
cy.get(
'#OverallMetricChart div[class*="rotatedVerticalBox"] button:eq(0)'
).contains(text1);
@@ -53,10 +50,7 @@ export function describeAxisConfigDialog(): void {
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.click();
cy.get("#AxisConfigPanel").find("button").contains("Apply").click();
cy.get(
'#OverallMetricChart div[class*="horizontalAxis"] button:eq(0)'
).contains(text1);
Original file line number Diff line number Diff line change
@@ -23,10 +23,7 @@ export function describeAxisConfigDialog(): void {
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.click();
cy.get("#AxisConfigPanel").find("button").contains("Apply").click();
cy.get(
'#OverallMetricChart div[class*="rotatedVerticalBox"] button:eq(0)'
).contains(text1);
@@ -53,10 +50,7 @@ export function describeAxisConfigDialog(): void {
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.click();
cy.get("#AxisConfigPanel").find("button").contains("Apply").click();
cy.get(
'#OverallMetricChart div[class*="horizontalAxis"] button:eq(0)'
).contains(text1);
10 changes: 8 additions & 2 deletions libs/core-ui/src/lib/Cohort/ShiftCohort/ShiftCohort.tsx
Original file line number Diff line number Diff line change
@@ -121,8 +121,14 @@ export class ShiftCohort extends React.Component<
temporaryCohort={this.state.savedCohorts[this.state.selectedCohort]}
/>
<DialogFooter>
<PrimaryButton onClick={this.onApplyClick} text="Apply" />
<DefaultButton onClick={this.props.onDismiss} text="Cancel" />
<PrimaryButton
onClick={this.onApplyClick}
text={localization.Core.ShiftCohort.apply}
/>
<DefaultButton
onClick={this.props.onDismiss}
text={localization.Core.ShiftCohort.cancel}
/>
</DialogFooter>
</Dialog>
);
4 changes: 2 additions & 2 deletions libs/core-ui/src/lib/components/AxisConfigDialog.tsx
Original file line number Diff line number Diff line change
@@ -151,7 +151,7 @@ export class AxisConfigDialog extends React.PureComponent<
onDismiss={this.props.onCancel}
isOpen
onRenderFooter={this.renderFooter}
isFooterAtBottom={false}
isFooterAtBottom
isLightDismiss
>
<Stack tokens={{ childrenGap: "l1" }}>
@@ -310,7 +310,7 @@ export class AxisConfigDialog extends React.PureComponent<
return (
<Stack horizontal tokens={{ childrenGap: "l1", padding: "l1" }}>
<PrimaryButton onClick={this.saveState}>
{localization.Interpret.AxisConfigDialog.select}
{localization.Interpret.AxisConfigDialog.apply}
</PrimaryButton>
<DefaultButton onClick={this.props.onCancel}>
{localization.Interpret.CohortEditor.cancel}
4 changes: 2 additions & 2 deletions libs/e2e/src/lib/describer/modelAssessment/Constants.ts
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@
// Licensed under the MIT License.

export enum Locators {
SelectButton = "button:contains('Select')",
ApplyButton = "button:contains('Apply')",
CancelButton = "button:contains('Cancel')",
YesButton = "button:contains('Yes')",
SaveAsNewCohortButton = "button:contains('Save as a new cohort')",
@@ -42,8 +42,8 @@ export enum Locators {
WICDatapointDropbox = "#IndividualFeatureContainer div[class^='ms-Stack legendAndText'] div[class^='ms-ComboBox-container']",
WICLocalImportanceDescription = "#LocalImportanceDescription",
WhatIfScatterChartYAxis = "#IndividualFeatureContainer div[class^='rotatedVerticalBox']",
WhatIfScatterChartFlyoutApply = "#AxisConfigPanel button:contains('Apply')",
WhatIfScatterChartFlyoutCancel = "#AxisConfigPanel button:contains('Cancel')",
WhatIfScatterChartFlyoutSelect = "#AxisConfigPanel button:contains('Select')",
WhatIfScatterChartSelectFeatureCaretButton = "#AxisConfigPanel i[data-icon-name='ChevronDown']",
WhatIfAxisPanel = "#AxisConfigPanel",
AxisFeatureDropdown = "#AxisConfigPanel div.ms-ComboBox-container",
Original file line number Diff line number Diff line change
@@ -45,7 +45,7 @@ export function describeAxisConfigDialog(
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.DECRotatedVerticalBox} button:eq(0)`).contains(
text1
);
@@ -54,7 +54,7 @@ export function describeAxisConfigDialog(
.click()
.get(`${Locators.DECChoiceFieldGroup} label:contains('Dataset')`)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
});
}
});
@@ -82,7 +82,7 @@ export function describeAxisConfigDialog(
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.DECHorizontalAxis} button:eq(0)`).contains(
text1
);
@@ -91,7 +91,7 @@ export function describeAxisConfigDialog(
.click()
.get(`${Locators.DECChoiceFieldGroup} label:contains('Index')`)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
});
}
});
Original file line number Diff line number Diff line change
@@ -85,6 +85,6 @@ export function axisSelection(label: string): void {
.click()
.get("#AxisConfigPanel")
.find("button")
.contains("Select")
.contains("Apply")
.click();
}
Original file line number Diff line number Diff line change
@@ -48,7 +48,7 @@ export function describeModelPerformanceBoxChart(
`${Locators.DECChoiceFieldGroup} label:contains(${dataShape.modelStatisticsData?.yAxisNewPanelValue})`
)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCRotatedVerticalBox}`).contains(
dataShape.modelStatisticsData?.yAxisNewValue || "age"
);
@@ -59,7 +59,7 @@ export function describeModelPerformanceBoxChart(
`${Locators.DECChoiceFieldGroup} label:contains(${dataShape.modelStatisticsData?.defaultYAxis})`
)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCRotatedVerticalBox}`).contains(
dataShape.modelStatisticsData?.defaultYAxis || "Cohort"
);
@@ -71,7 +71,7 @@ export function describeModelPerformanceBoxChart(
.invoke("text")
.then((text1) => {
cy.get(`#AxisConfigPanel label:contains(${text1})`).click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCHorizontalAxis} button:eq(0)`).contains(text1);
});
cy.get(`${Locators.MSCHorizontalAxis} button`)
@@ -80,7 +80,7 @@ export function describeModelPerformanceBoxChart(
`${Locators.DECChoiceFieldGroup} label:contains(${dataShape.modelStatisticsData?.defaultXAxisPanelValue})`
)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCHorizontalAxis}`).contains(
dataShape.modelStatisticsData?.xAxisNewValue || ""
);
Original file line number Diff line number Diff line change
@@ -39,7 +39,7 @@ export function describeModelPerformanceSideBar(
}
});

cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCRotatedVerticalBox}`).contains(
dataShape.modelStatisticsData?.yAxisNewValue || "age"
);
@@ -56,7 +56,7 @@ export function describeModelPerformanceSideBar(
`${Locators.DECChoiceFieldGroup} label:contains(${dataShape.modelStatisticsData?.defaultYAxis})`
)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCRotatedVerticalBox}`).contains(
dataShape.modelStatisticsData?.defaultYAxis || "Cohort"
);
@@ -76,7 +76,7 @@ export function describeModelPerformanceSideBar(
`${Locators.DECChoiceFieldGroup} label:contains(${dataShape.modelStatisticsData?.yAxisNewPanelValue})`
)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCRotatedVerticalBox}`).contains(
dataShape.modelStatisticsData?.yAxisNewValue || "age"
);
@@ -92,7 +92,7 @@ export function describeModelPerformanceSideBar(
`${Locators.DECChoiceFieldGroup} label:contains(${dataShape.modelStatisticsData?.defaultYAxis})`
)
.click();
cy.get(Locators.SelectButton).click();
cy.get(Locators.ApplyButton).click();
cy.get(`${Locators.MSCRotatedVerticalBox}`).contains(
dataShape.modelStatisticsData?.defaultYAxis || "Cohort"
);
Original file line number Diff line number Diff line change
@@ -36,7 +36,7 @@ export function describeAxisFlyouts(dataShape: IModelAssessmentData): void {
cy.get(
`${Locators.AxisFeatureDropdownOptionGeneral} button:contains('${dataShape.whatIfCounterfactualsData?.yAxisNewValue}')`
).click();
cy.get(Locators.WhatIfScatterChartFlyoutSelect).click();
cy.get(Locators.WhatIfScatterChartFlyoutApply).click();
cy.get(
`${Locators.WhatIfScatterChartYAxis} button:contains('${dataShape.whatIfCounterfactualsData?.yAxisNewValue}')`
).should("exist");
@@ -45,7 +45,7 @@ export function describeAxisFlyouts(dataShape: IModelAssessmentData): void {
it("should be able to select axis value", () => {
cy.get(Locators.WhatIfScatterChartYAxis).click();
cy.get(Locators.WhatIfYAxisAxisValueNewValue).click();
cy.get(Locators.WhatIfScatterChartFlyoutSelect).click();
cy.get(Locators.WhatIfScatterChartFlyoutApply).click();
cy.get(Locators.WhatIfScatterChartYAxisLabelUpdated2).should("exist");
});
});
@@ -79,7 +79,7 @@ export function describeAxisFlyouts(dataShape: IModelAssessmentData): void {
cy.get(
`${Locators.AxisFeatureDropdownOptionGeneral} button:contains('${dataShape.whatIfCounterfactualsData?.newClassValue}')`
).click();
cy.get(Locators.WhatIfScatterChartFlyoutSelect).click();
cy.get(Locators.WhatIfScatterChartFlyoutApply).click();
cy.get(
`${Locators.WhatIfScatterChartXAxisLabelUpdatedGeneral} button:contains('${dataShape.whatIfCounterfactualsData?.newClassValue}')`
).should("exist");
@@ -89,7 +89,7 @@ export function describeAxisFlyouts(dataShape: IModelAssessmentData): void {
it("should be able to select axis value", () => {
cy.get(Locators.WhatIfScatterChartXAxis).click();
cy.get(Locators.WhatIfXAxisAxisValueNewValue).click();
cy.get(Locators.WhatIfScatterChartFlyoutSelect).click();
cy.get(Locators.WhatIfScatterChartFlyoutApply).click();
cy.get(Locators.WhatIfScatterChartXAxisLabelUpdated2).should("exist");
});
});
Original file line number Diff line number Diff line change
@@ -164,6 +164,8 @@ export class FeatureList extends React.Component<
closeButtonAriaLabel="Close"
isBlocking={false}
onDismiss={this.props.onDismiss}
onRenderFooterContent={this.renderPanelFooter}
isFooterAtBottom
>
<div className="featuresSelector">
<Stack tokens={checkboxStackTokens} verticalAlign="space-around">
@@ -246,18 +248,6 @@ export class FeatureList extends React.Component<
isEnabled={this.props.isEnabled}
/>
</Stack.Item>
{this.props.isEnabled && (
// Remove apply button in static view
<Stack.Item key="applyButtonKey" align="start">
<PrimaryButton
text="Apply"
onClick={this.applyClick}
allowDisabledFocus
disabled={!this.state.enableApplyButton}
checked={false}
/>
</Stack.Item>
)}
</Stack>
</div>
</Panel>
@@ -334,6 +324,24 @@ export class FeatureList extends React.Component<
return <span />;
}

private readonly renderPanelFooter = () => {
// Remove apply button in static view
if (!this.props.isEnabled) {
return <span />;
}
return (
<Stack.Item key="applyButtonKey" align="start">
<PrimaryButton
text={localization.ErrorAnalysis.FeatureList.apply}
onClick={this.applyClick}
allowDisabledFocus
disabled={!this.state.enableApplyButton}
checked={false}
/>
</Stack.Item>
);
};

private updateSelection(): void {
this._selection.setItems(this.state.tableState.rows);
const featureNames = this.state.tableState.rows.map((row) => row[0]);
7 changes: 5 additions & 2 deletions libs/localization/src/lib/en.json
Original file line number Diff line number Diff line change
@@ -84,6 +84,8 @@
"Unknown": "unknown"
},
"ShiftCohort": {
"apply": "Apply",
"cancel": "Cancel",
"title": "Switch Cohort",
"subText": "Select a cohort from the cohort list. Apply the cohort to the dashboard."
},
@@ -180,6 +182,7 @@
"subText": "Learn about the selected cohort. Edit its cohort name. Delete this cohort."
},
"FeatureList": {
"apply": "Apply",
"features": "Features",
"importances": "Importances",
"treeMapDescription": "To retrain the tree map, select and save the features below. The feature importances were calculated using mutual information with the error on the true labels. Please use it as a guideline for training the tree map.",
@@ -719,12 +722,12 @@
"_selectClass.comment": "label for dropdown listing all classes",
"_selectFeature.comment": "dropdown label to select feature (column) for charting",
"_selectFilter.comment": "label on dropdown to pick value for charting",
"apply": "Apply",
"binLabel": "Apply binning to data",
"countHelperText": "A histogram of the number of points",
"ditherLabel": "Should dither",
"groupByCohort": "Group by cohort",
"numOfBins": "Number of bins",
"select": "Select",
"selectClass": "Select class",
"selectFeature": "Select feature",
"selectFilter": "Select your axis value"
@@ -1415,7 +1418,7 @@
"outlierLabel": "Outliers",
"boxPlotSeriesLabel": "Box Plot"
},
"chartConfigConfirm": "Confirm",
"chartConfigApply": "Apply",
"chartConfigCancel": "Cancel",
"chartConfigDatasetCohortSelectionPlaceholder": "Select dataset cohorts",
"chartConfigFeatureBasedCohortSelectionPlaceholder": "Select feature-based cohorts",
Original file line number Diff line number Diff line change
@@ -281,7 +281,7 @@ export class ChartConfigurationFlyout extends React.Component<
<Stack horizontal tokens={{ childrenGap: "10px" }}>
<PrimaryButton
onClick={this.onConfirm}
text={localization.ModelAssessment.ModelOverview.chartConfigConfirm}
text={localization.ModelAssessment.ModelOverview.chartConfigApply}
disabled={this.noCohortIsSelected()}
/>
<DefaultButton
Loading

0 comments on commit 94dd072

Please sign in to comment.