Fix reset color #3237
Fix reset color #3237
Conversation
| @@ -38,6 +38,11 @@ const ColorFooter: React.FC = () => { | |||
| return true; | |||
| }, [savedSettings]); | |||
|
|
|||
| // set to be true only for teams | |||
| const isTeams = useRecoilValue(fos.compactLayout); | |||
There was a problem hiding this comment.
This is logic/naming something we should avoid. compactLayout means isTeams by coincidence only
| @@ -47,7 +52,7 @@ const ColorFooter: React.FC = () => { | |||
| text={"Reset"} | |||
| title={`Clear session settings and revert to default settings`} | |||
| onClick={() => { | |||
| setColorScheme(false, null); | |||
| setColorScheme(false, isTeams ? datasetDefault : null); | |||
There was a problem hiding this comment.
I'm not sure I understand the behavior here. Reset means two different things based on the context? How does a Teams user nullify a dataset default? Ideally we add a selector that resolves to the reset value (resolution should be opaque/decoupled)
There was a problem hiding this comment.
Reset always means reset to dataset default (if there is one). We have something else to clear dataset default setting.
By setting colorScheme to null, OSS will load dataset default or color pool from fo.app_config or session.config from the session. But teams can't update through the session. Therefore, I am pointing the setting to datasetDefault here for TEAMS.
There was a problem hiding this comment.
Ok. In my routing work, the color scheme will update through a session regardless of what the means to the context (Teams, OSS, or a hypothetical). I'm just advocating that this doesn't leak into OSS and component logic
What changes are proposed in this pull request?
resetbutton failed to setcan viewpermission user setting back to dataset app_config setting on TEAMS.How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changes