KEMBAR78
Fix "Card Width" property reset button. by bmd3k · Pull Request #5841 · tensorflow/tensorboard · GitHub
Skip to content

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.

2 participants