Skip to content

Commit 896c778

Browse files
authored
[Global pins] Add clear all pins button (#6828)
## Motivation for features / changes Following #6821 As pin data is accumulated, unwanted pins can easily crowd the dashboard. Therefore, this PR adds a button that allows users to conveniently clear all pins at once. ## Technical description of changes * 57c6a47 Introduced new action called `metricsClearAllPinnedCards` * When `metricsClearAllPinnedCards` is dispatched, the reducer removed all pinned cards in the state. * a04c2d7 Added `clear all pins` button in the pinned view container. * The button will be shown if there's at least one pinned card. * f0c3a54 Implemented `removeAllScalarPins` method in the saved pins data source. * This method removes stored pins from local storage. * e97b110 Added `removeAllPins` effect in the `metrics/effects/index.ts` * When `metricsClearAllPinnedCards` action is called, this effect will call `removeAllScalarPins` method defined in the saved pin data source. * c0edd37 guarded UI feature (button) with feature flag and added related tests. ## Screenshots of UI changes (or N/A) ### Light mode ![image](https://github.com/tensorflow/tensorboard/assets/24772412/a0793e8b-7fea-45f0-b2d8-4e742ca40918) ### Dark mode ![image](https://github.com/tensorflow/tensorboard/assets/24772412/581468a1-1a9c-4ab4-b70a-13c5a5b168f6) ## Detailed steps to verify changes work correctly (as executed by you) Unit test pass & cl TAP presubmit pass ## Alternate designs / implementations considered (or N/A) I also considered a feature to select a pinned card and remove the selected pinned card, but I thought that this would be the same as pressing the 'unpin' button individually from the user's perspective. So I implemented a 'clear all pins' button that removes all pinned cards. Any feedback is appreciated!
1 parent 6f1cbcb commit 896c778

12 files changed

+288
-19
lines changed

tensorboard/webapp/metrics/actions/index.ts

+4
Original file line numberDiff line numberDiff line change
@@ -277,5 +277,9 @@ export const metricsUnresolvedPinnedCardsFromLocalStorageAdded = createAction(
277277
props<{cards: CardUniqueInfo[]}>()
278278
);
279279

280+
export const metricsClearAllPinnedCards = createAction(
281+
'[Metrics] Clear all pinned cards'
282+
);
283+
280284
// TODO(jieweiwu): Delete after internal code is updated.
281285
export const stepSelectorTimeSelectionChanged = timeSelectionChanged;

tensorboard/webapp/metrics/data_source/saved_pins_data_source.ts

+4
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,8 @@ export class SavedPinsDataSource {
4545
}
4646
return [];
4747
}
48+
49+
removeAllScalarPins(): void {
50+
window.localStorage.setItem(SAVED_SCALAR_PINS_KEY, JSON.stringify([]));
51+
}
4852
}

tensorboard/webapp/metrics/data_source/saved_pins_data_source_test.ts

+11
Original file line numberDiff line numberDiff line change
@@ -114,4 +114,15 @@ describe('SavedPinsDataSource Test', () => {
114114
expect(dataSource.getSavedScalarPins()).toEqual(['tag1']);
115115
});
116116
});
117+
118+
describe('removeAllScalarPins', () => {
119+
it('removes all existing pins', () => {
120+
dataSource.saveScalarPin('tag3');
121+
dataSource.saveScalarPin('tag4');
122+
123+
dataSource.removeAllScalarPins();
124+
125+
expect(dataSource.getSavedScalarPins().length).toEqual(0);
126+
});
127+
});
117128
});

tensorboard/webapp/metrics/effects/index.ts

+20-1
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,21 @@ export class MetricsEffects implements OnInitEffects {
316316
})
317317
);
318318

319+
private readonly removeAllPins$ = this.actions$.pipe(
320+
ofType(actions.metricsClearAllPinnedCards),
321+
withLatestFrom(
322+
this.store.select(selectors.getEnableGlobalPins),
323+
this.store.select(selectors.getShouldPersistSettings)
324+
),
325+
filter(
326+
([, enableGlobalPins, shouldPersistSettings]) =>
327+
enableGlobalPins && shouldPersistSettings
328+
),
329+
tap(() => {
330+
this.savedPinsDataSource.removeAllScalarPins();
331+
})
332+
);
333+
319334
/**
320335
* In general, this effect dispatch the following actions:
321336
*
@@ -356,7 +371,11 @@ export class MetricsEffects implements OnInitEffects {
356371
/**
357372
* Subscribes to: dashboard shown (initAction).
358373
*/
359-
this.loadSavedPins$
374+
this.loadSavedPins$,
375+
/**
376+
* Subscribes to: metricsClearAllPinnedCards.
377+
*/
378+
this.removeAllPins$
360379
);
361380
},
362381
{dispatch: false}

tensorboard/webapp/metrics/effects/metrics_effects_test.ts

+37
Original file line numberDiff line numberDiff line change
@@ -985,5 +985,42 @@ describe('metrics effects', () => {
985985
expect(actualActions).toEqual([]);
986986
});
987987
});
988+
989+
describe('removeAllPins', () => {
990+
let removeAllScalarPinsSpy: jasmine.Spy;
991+
992+
beforeEach(() => {
993+
removeAllScalarPinsSpy = spyOn(
994+
savedPinsDataSource,
995+
'removeAllScalarPins'
996+
);
997+
store.overrideSelector(selectors.getEnableGlobalPins, true);
998+
store.refreshState();
999+
});
1000+
1001+
it('removes all pins by calling removeAllScalarPins method', () => {
1002+
actions$.next(actions.metricsClearAllPinnedCards());
1003+
1004+
expect(removeAllScalarPinsSpy).toHaveBeenCalled();
1005+
});
1006+
1007+
it('does not remove pins if getEnableGlobalPins is false', () => {
1008+
store.overrideSelector(selectors.getEnableGlobalPins, false);
1009+
store.refreshState();
1010+
1011+
actions$.next(actions.metricsClearAllPinnedCards());
1012+
1013+
expect(removeAllScalarPinsSpy).not.toHaveBeenCalled();
1014+
});
1015+
1016+
it('does not remove pins if getShouldPersistSettings is false', () => {
1017+
store.overrideSelector(selectors.getShouldPersistSettings, false);
1018+
store.refreshState();
1019+
1020+
actions$.next(actions.metricsClearAllPinnedCards());
1021+
1022+
expect(removeAllScalarPinsSpy).not.toHaveBeenCalled();
1023+
});
1024+
});
9881025
});
9891026
});

tensorboard/webapp/metrics/store/metrics_reducers.ts

+24-1
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ import {
7777
TagMetadata,
7878
TimeSeriesData,
7979
TimeSeriesLoadable,
80+
CardToPinnedCard,
81+
PinnedCardToCard,
8082
} from './metrics_types';
8183
import {dataTableUtils} from '../../widgets/data_table/utils';
8284

@@ -1525,7 +1527,28 @@ const reducer = createReducer(
15251527
],
15261528
};
15271529
}
1528-
)
1530+
),
1531+
on(actions.metricsClearAllPinnedCards, (state) => {
1532+
const nextCardMetadataMap = {...state.cardMetadataMap};
1533+
const nextCardStepIndex = {...state.cardStepIndex};
1534+
const nextCardStateMap = {...state.cardStateMap};
1535+
1536+
for (const cardId of state.pinnedCardToOriginal.keys()) {
1537+
delete nextCardMetadataMap[cardId];
1538+
delete nextCardStepIndex[cardId];
1539+
delete nextCardStateMap[cardId];
1540+
}
1541+
1542+
return {
1543+
...state,
1544+
cardMetadataMap: nextCardMetadataMap,
1545+
cardStateMap: nextCardStateMap,
1546+
cardStepIndex: nextCardStepIndex,
1547+
cardToPinnedCopy: new Map() as CardToPinnedCard,
1548+
cardToPinnedCopyCache: new Map() as CardToPinnedCard,
1549+
pinnedCardToOriginal: new Map() as PinnedCardToCard,
1550+
};
1551+
})
15291552
);
15301553

15311554
export function reducers(state: MetricsState | undefined, action: Action) {

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

+52
Original file line numberDiff line numberDiff line change
@@ -4488,5 +4488,57 @@ describe('metrics reducers', () => {
44884488
expect(state2.unresolvedImportedPinnedCards).toEqual([fakePinnedCard]);
44894489
});
44904490
});
4491+
4492+
describe('#metricsClearAllPinnedCards', () => {
4493+
it('unpins all pinned cards', () => {
4494+
const beforeState = buildMetricsState({
4495+
cardMetadataMap: {
4496+
card1: createScalarCardMetadata(),
4497+
pinnedCopy1: createScalarCardMetadata(),
4498+
card2: createScalarCardMetadata(),
4499+
pinnedCopy2: createScalarCardMetadata(),
4500+
},
4501+
cardList: ['card1', 'card2'],
4502+
cardStepIndex: {
4503+
card1: buildStepIndexMetadata({index: 10}),
4504+
pinnedCopy1: buildStepIndexMetadata({index: 20}),
4505+
card2: buildStepIndexMetadata({index: 11}),
4506+
pinnedCopy2: buildStepIndexMetadata({index: 21}),
4507+
},
4508+
cardToPinnedCopy: new Map([
4509+
['card1', 'pinnedCopy1'],
4510+
['card2', 'pinnedCopy2'],
4511+
]),
4512+
cardToPinnedCopyCache: new Map([
4513+
['card1', 'pinnedCopy1'],
4514+
['card2', 'pinnedCopy2'],
4515+
]),
4516+
pinnedCardToOriginal: new Map([
4517+
['pinnedCopy1', 'card1'],
4518+
['pinnedCopy2', 'card2'],
4519+
]),
4520+
});
4521+
const nextState = reducers(
4522+
beforeState,
4523+
actions.metricsClearAllPinnedCards()
4524+
);
4525+
4526+
const expectedState = buildMetricsState({
4527+
cardMetadataMap: {
4528+
card1: createScalarCardMetadata(),
4529+
card2: createScalarCardMetadata(),
4530+
},
4531+
cardList: ['card1', 'card2'],
4532+
cardStepIndex: {
4533+
card1: buildStepIndexMetadata({index: 10}),
4534+
card2: buildStepIndexMetadata({index: 11}),
4535+
},
4536+
cardToPinnedCopy: new Map(),
4537+
cardToPinnedCopyCache: new Map(),
4538+
pinnedCardToOriginal: new Map(),
4539+
});
4540+
expect(nextState).toEqual(expectedState);
4541+
});
4542+
});
44914543
});
44924544
});

tensorboard/webapp/metrics/testing.ts

+2
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,8 @@ export class TestingSavedPinsDataSource {
405405
getSavedScalarPins() {
406406
return [];
407407
}
408+
409+
removeAllScalarPins() {}
408410
}
409411

410412
export function provideTestingSavedPinsDataSource() {

tensorboard/webapp/metrics/views/main_view/main_view_test.ts

+67-1
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ import {MainViewComponent, SHARE_BUTTON_COMPONENT} from './main_view_component';
6969
import {MainViewContainer} from './main_view_container';
7070
import {PinnedViewComponent} from './pinned_view_component';
7171
import {PinnedViewContainer} from './pinned_view_container';
72+
import {buildMockState} from '../../../testing/utils';
7273

7374
@Component({
7475
selector: 'card-view',
@@ -182,7 +183,11 @@ describe('metrics main view', () => {
182183
],
183184
providers: [
184185
provideMockStore({
185-
initialState: appStateFromMetricsState(buildMetricsState()),
186+
initialState: {
187+
...buildMockState({
188+
...appStateFromMetricsState(buildMetricsState()),
189+
}),
190+
},
186191
}),
187192
],
188193
// Skip errors for card renderers, which are tested separately.
@@ -1606,6 +1611,67 @@ describe('metrics main view', () => {
16061611
expect(indicator).toBeTruthy();
16071612
});
16081613
});
1614+
1615+
describe('clear all pins button', () => {
1616+
beforeEach(() => {
1617+
store.overrideSelector(selectors.getEnableGlobalPins, true);
1618+
});
1619+
1620+
it('does not show the button if getEnableGlobalPins is false', () => {
1621+
store.overrideSelector(selectors.getEnableGlobalPins, false);
1622+
store.overrideSelector(selectors.getPinnedCardsWithMetadata, []);
1623+
const fixture = TestBed.createComponent(MainViewContainer);
1624+
fixture.detectChanges();
1625+
1626+
const clearAllButton = fixture.debugElement.query(
1627+
By.css('[aria-label="Clear all pinned cards"]')
1628+
);
1629+
expect(clearAllButton).toBeNull();
1630+
});
1631+
1632+
it('does not show the button if there is no pinned card', () => {
1633+
store.overrideSelector(selectors.getPinnedCardsWithMetadata, []);
1634+
const fixture = TestBed.createComponent(MainViewContainer);
1635+
fixture.detectChanges();
1636+
1637+
const clearAllButton = fixture.debugElement.query(
1638+
By.css('[aria-label="Clear all pinned cards"]')
1639+
);
1640+
expect(clearAllButton).toBeNull();
1641+
});
1642+
1643+
it('shows the button if there is a pinned card', () => {
1644+
store.overrideSelector(selectors.getPinnedCardsWithMetadata, [
1645+
{cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
1646+
{cardId: 'card2', ...createCardMetadata(PluginType.IMAGES)},
1647+
]);
1648+
const fixture = TestBed.createComponent(MainViewContainer);
1649+
fixture.detectChanges();
1650+
1651+
const clearAllButton = fixture.debugElement.query(
1652+
By.css('[aria-label="Clear all pinned cards"]')
1653+
);
1654+
expect(clearAllButton).toBeTruthy();
1655+
});
1656+
1657+
it('dispatch clear all action when the button is clicked', () => {
1658+
store.overrideSelector(selectors.getPinnedCardsWithMetadata, [
1659+
{cardId: 'card1', ...createCardMetadata(PluginType.SCALARS)},
1660+
{cardId: 'card2', ...createCardMetadata(PluginType.IMAGES)},
1661+
]);
1662+
const fixture = TestBed.createComponent(MainViewContainer);
1663+
fixture.detectChanges();
1664+
1665+
const clearAllButton = fixture.debugElement.query(
1666+
By.css('[aria-label="Clear all pinned cards"]')
1667+
);
1668+
clearAllButton.nativeElement.click();
1669+
1670+
expect(dispatchedActions).toEqual([
1671+
actions.metricsClearAllPinnedCards(),
1672+
]);
1673+
});
1674+
});
16091675
});
16101676

16111677
describe('slideout menu', () => {

tensorboard/webapp/metrics/views/main_view/pinned_view_component.scss

+19
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,25 @@ mat-icon {
2626
margin-right: 5px;
2727
}
2828

29+
.group-toolbar {
30+
justify-content: space-between;
31+
}
32+
33+
.left-items {
34+
display: flex;
35+
align-items: center;
36+
}
37+
38+
.right-items {
39+
button {
40+
$_height: 25px;
41+
font-size: 12px;
42+
font-weight: normal;
43+
height: $_height;
44+
line-height: $_height;
45+
}
46+
}
47+
2948
.group-text {
3049
display: flex;
3150
align-items: baseline;

0 commit comments

Comments
 (0)