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

Time Series: tooltip sort by alphabetical order #5442

Merged
merged 7 commits into from
Dec 1, 2021

Conversation

japie1235813
Copy link
Contributor

@japie1235813 japie1235813 commented Nov 30, 2021

In the list of tooltip sorting method, we remove "default" and replace with "alphabetical order". The alphabetical order is based on the display name.

Screen Shot 2021-11-30 at 1 44 54 PM

Screen Shot 2021-11-30 at 10 38 22 AM

@google-cla google-cla bot added the cla: yes label Nov 30, 2021
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

Cool, could you tell me whether changing the setting is "sticky" and is remembered? Also, we may need some migration in case {tooltipSort: "default"} is set on local storage to {tooltipSort: "alphabetical"}.

@@ -27,7 +27,7 @@ export enum PluginType {
// When editing a value of the enum, please write a backward compatible
// deserializer in data_source/metrics_data_source.ts.
export enum TooltipSort {
DEFAULT = 'default',
ALPHABETICAL = 'Alphabetical',
Copy link
Contributor

Choose a reason for hiding this comment

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

ALPHABETICAL = 'alphabetical', for consistency please.

@japie1235813
Copy link
Contributor Author

Yes, it's preserved in local storage
Screen Shot 2021-11-30 at 5 20 00 PM
.

for {tooltipSort: "default"} under that case the sorting method would be the default (which is alphabetical).
Do you feel we need to explicitly change the value in local storage to {tooltipSort: "alphabetical"}?
(I'm not quite sure how to manually update the local storage when loading, try a couple of things but does not work out)

In new commit I add DEFAULT back and fall to alphabetic when loading the global settings.

Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

Tested manually by altering the local storage. Looks like because of the switch statement in the metrics_reducers, everything work as intended. LGTM.

Comment on lines 378 to 379
metricsSettings.tooltipSort = TooltipSort.ALPHABETICAL;
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

alternative: remove these two lines.

@japie1235813 japie1235813 merged commit 96c8faa into tensorflow:master Dec 1, 2021
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
In the list of tooltip sorting method, we replace "default" with "alphabetical". The alphabetical order is based on the display name.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
In the list of tooltip sorting method, we replace "default" with "alphabetical". The alphabetical order is based on the display name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants