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

Suggested Cards: Create data source for persisting card interactions to localStorage #6439

Merged

Conversation

rileyajones
Copy link
Contributor

@rileyajones rileyajones commented Jun 16, 2023

Motivation for features / changes

We are working on a feature to suggest cards to users at the top of the timeseries dashboard. Suggestions are generated based on previous interactions with TensorBoard. In order to determine what those previous interactions were we are storing them to localStorage.
See #6437 for more information.
Googlers see go/tb-suggested-cards

@rileyajones rileyajones requested a review from JamesHollyer June 16, 2023 19:14
@rileyajones rileyajones marked this pull request as ready for review June 16, 2023 19:14
@rileyajones rileyajones removed the request for review from JamesHollyer June 20, 2023 20:56
@rileyajones rileyajones marked this pull request as draft June 20, 2023 20:56
@rileyajones rileyajones force-pushed the card-interactions-datasource branch from fc6494d to e15cd4d Compare August 30, 2023 22:05
@rileyajones rileyajones force-pushed the card-interactions-datasource branch from e15cd4d to 594e291 Compare August 30, 2023 22:08
@rileyajones rileyajones marked this pull request as ready for review August 30, 2023 22:51
@@ -453,6 +453,16 @@ const {initialState, reducers: namespaceContextedReducer} =
settings: METRICS_SETTINGS_DEFAULT,
settingOverrides: {},
visibleCardMap: new Map<ElementId, CardId>(),
previousCardInteractions: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine. However, I would prefer to add this state into the reducer when it is starting to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would need to define the state property as optional in order to make typescript happy and I would rather not do that. It's also inconsistent with the way we define other defaults.

@rileyajones rileyajones merged commit 81b1ab0 into tensorflow:master Sep 14, 2023
13 checks passed
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.

2 participants