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

Fix "Card Width" property reset button. #5841

Merged
merged 2 commits into from Aug 4, 2022

Conversation

bmd3k
Copy link
Contributor

@bmd3k bmd3k commented Aug 4, 2022

The reset button for "Card Width" does not effectively reset the property. Pressing it resets it to the value that it was set to when the page was loaded. (So, for example, if the value is 535px on page load, pressing reset will reset it to 535px and not reset the property entirely).

The reasoning is:

We fix the problem by setting the state value to null rather than removing it entirely. This means it will also be set to null in local storage and effectively reset the property.

@bmd3k bmd3k requested a review from japie1235813 August 4, 2022 16:08
Copy link
Contributor

@japie1235813 japie1235813 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. In addition to this PR, do we also need to clean up the value stored in local storage?

@bmd3k
Copy link
Contributor Author

bmd3k commented Aug 4, 2022

Thanks for the fix. In addition to this PR, do we also need to clean up the value stored in local storage?

I don't think so. We can't identify or cleanup users who've tried to reset in the past. But the next time they press reset it should just work.

@bmd3k bmd3k merged commit ec48bc1 into tensorflow:master Aug 4, 2022
yatbear pushed a commit that referenced this pull request Aug 5, 2022
The reset button for "Card Width" does not effectively reset the property. Pressing it resets it to the value that it was set to when the page was loaded. (So, for example, if the value is 535px on page load, pressing reset will reset it to 535px and not reset the property entirely).

The reasoning is:
* Pressing the reset button will remove the property from state, effectively making it undefined:
  * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/metrics/store/metrics_reducers.ts;l=709;drc=2059b53ef0f6875dbaa8bca4362653c07d0069fd
* The PersistentSettingsDataSource does not update the field in local storage if the value is undefined:
  * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts;l=87;drc=54be1df836817d1a5a6378902eec23a9fe5cb40d
* And, so, the value will continue to be whatever was stored in local storage on page load. 

We fix the problem by setting the state value to null rather than removing it entirely. This means it will also be set to null in local storage and effectively reset the property.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
The reset button for "Card Width" does not effectively reset the property. Pressing it resets it to the value that it was set to when the page was loaded. (So, for example, if the value is 535px on page load, pressing reset will reset it to 535px and not reset the property entirely).

The reasoning is:
* Pressing the reset button will remove the property from state, effectively making it undefined:
  * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/metrics/store/metrics_reducers.ts;l=709;drc=2059b53ef0f6875dbaa8bca4362653c07d0069fd
* The PersistentSettingsDataSource does not update the field in local storage if the value is undefined:
  * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts;l=87;drc=54be1df836817d1a5a6378902eec23a9fe5cb40d
* And, so, the value will continue to be whatever was stored in local storage on page load. 

We fix the problem by setting the state value to null rather than removing it entirely. This means it will also be set to null in local storage and effectively reset the property.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
The reset button for "Card Width" does not effectively reset the property. Pressing it resets it to the value that it was set to when the page was loaded. (So, for example, if the value is 535px on page load, pressing reset will reset it to 535px and not reset the property entirely).

The reasoning is:
* Pressing the reset button will remove the property from state, effectively making it undefined:
  * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/metrics/store/metrics_reducers.ts;l=709;drc=2059b53ef0f6875dbaa8bca4362653c07d0069fd
* The PersistentSettingsDataSource does not update the field in local storage if the value is undefined:
  * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts;l=87;drc=54be1df836817d1a5a6378902eec23a9fe5cb40d
* And, so, the value will continue to be whatever was stored in local storage on page load. 

We fix the problem by setting the state value to null rather than removing it entirely. This means it will also be set to null in local storage and effectively reset the property.
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.

None yet

2 participants