[Global pins] Add clear all pins button #6828
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roseayeon
force-pushed
the
clear-all-pins
branch
from
05f0fd0
to
d16ba83
Compare
roseayeon
changed the title
hoonji
approved these changes
Apr 12, 2024
There was a problem hiding this comment.
Thanks for the super clean code, it was very easy to review!
One suggestion: maybe it's better to use an icon for the button instead, as most buttons on the dashboard are icons? I think a block icon could be very intuitive and take up less space.
But the current version is also very clean too so I think we can maybe gather other opinions in a teamfood first.
Thanks for the idea, @hoonji ! I'll leave it as is for now and gather feedback from the team after the feature is fully implemented. (If you have any other comments please feel free to share them! ccing @bmd3k ) |
1 task
roseayeon
added a commit
that referenced
this pull request
Apr 19, 2024
## Motivation for features / changes Following #6828, Since global pins are applied automatically, this PR allows users disable the global pin feature. ## Technical description of changes 2cf2b11 Added `savingPinsEnabled` in the `MetricsSettings` * Also added a selector getting `settings.savingPinsEnabled` value from the store. dec6a63 Introduced new action `metricsEnableSavingPinsToggled` * If `metricsEnableSavingPinsToggled` is dispatched, it toggles the `savingPinsEnabled` store value. 6ea22fd Added a checkbox UI to the settings view component. * Also added a feature flag guard to the component. If check box is clicked, it dispatched `metricsEnableSavingPinsToggled` action. 568febf Added a new effect `disableSavingPins` * When `metricsEnableSavingPinsToggled` action is called, this effect will call `removeAllScalarPins` method if `savingPinsEnabled` value is false. bbf9050 Added saving pins feature in persistent settings to preserve user preferences. 3ff7cd2 Add buildMetricsSettingsOverrides util in the testing to use in tbcorp ## Screenshots of UI changes (or N/A)  ## Detailed steps to verify changes work correctly (as executed by you) Unit test passes + created a cl/625208577 to pass TAP Presubmit tests ## Alternate designs / implementations considered (or N/A) N/A ## Action Items in the next PR In the next PR, I'll add a "confirmation dialog" that appears when the user unchecks the checkbox to make the user aware that local storage is disappearing. - [ ] Add the confirmation dialog
AnuarTB
pushed a commit
to AnuarTB/tensorboard
that referenced
this pull request
May 3, 2024
## Motivation for features / changes Following tensorflow#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  ### Dark mode  ## 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!
AnuarTB
pushed a commit
to AnuarTB/tensorboard
that referenced
this pull request
May 3, 2024
## Motivation for features / changes Following tensorflow#6828, Since global pins are applied automatically, this PR allows users disable the global pin feature. ## Technical description of changes 2cf2b11 Added `savingPinsEnabled` in the `MetricsSettings` * Also added a selector getting `settings.savingPinsEnabled` value from the store. dec6a63 Introduced new action `metricsEnableSavingPinsToggled` * If `metricsEnableSavingPinsToggled` is dispatched, it toggles the `savingPinsEnabled` store value. 6ea22fd Added a checkbox UI to the settings view component. * Also added a feature flag guard to the component. If check box is clicked, it dispatched `metricsEnableSavingPinsToggled` action. 568febf Added a new effect `disableSavingPins` * When `metricsEnableSavingPinsToggled` action is called, this effect will call `removeAllScalarPins` method if `savingPinsEnabled` value is false. bbf9050 Added saving pins feature in persistent settings to preserve user preferences. 3ff7cd2 Add buildMetricsSettingsOverrides util in the testing to use in tbcorp ## Screenshots of UI changes (or N/A)  ## Detailed steps to verify changes work correctly (as executed by you) Unit test passes + created a cl/625208577 to pass TAP Presubmit tests ## Alternate designs / implementations considered (or N/A) N/A ## Action Items in the next PR In the next PR, I'll add a "confirmation dialog" that appears when the user unchecks the checkbox to make the user aware that local storage is disappearing. - [ ] Add the confirmation dialog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
metricsClearAllPinnedCardsmetricsClearAllPinnedCardsis dispatched, the reducer removed all pinned cards in the state.a04c2d7 Added
clear all pinsbutton in the pinned view container.f0c3a54 Implemented
removeAllScalarPinsmethod in the saved pins data source.e97b110 Added
removeAllPinseffect in themetrics/effects/index.tsmetricsClearAllPinnedCardsaction is called, this effect will callremoveAllScalarPinsmethod 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
Dark mode
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!