-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
CSF: Allow overridding globals at the story level #26654
Changes from 29 commits
2588fbd
2c09577
289f825
2e4ffd4
bd5e5d6
3f5b00c
ef80953
2786bc8
5356849
d94b58c
fcc3c02
3338701
84def45
57ac8f6
8bb1298
f96f934
866dfd3
425e870
c11a299
48d06e5
73cb16b
cc72801
ce5b681
add0674
ef8c846
20220e8
20a82fa
b2c4b40
f7b3464
c05dcb2
21cfe8b
04919f7
25db51b
20efb2a
1036b0e
a1a6a7d
c1b7e87
c95544d
fd9d26f
6df5b90
45c88ce
80e7002
8920bb6
3aecb76
f7d02ae
0232e45
0356095
f122c3b
2a43164
0cd87ad
861dd65
971abc4
9d1525a
9b3ceb2
612a98a
599cce8
6daacfa
5b82a2e
9c5fc9f
997efa3
32a2d3d
999b619
4f9967d
6eaeb74
b3669ab
eba7183
301af1c
88ab1a2
c22499b
3d312d5
248893d
cafeaff
44163f5
3254d1b
df42f65
453dfd6
7e53f9e
ee3f55c
9f8bc38
3aafff7
b7df747
1adbd1c
94d7ab2
214d423
86db1e6
a14b228
e92e313
1b34ca2
91f9ccb
d502f22
9fb86c5
3e8fc2c
9eedc1b
a8f9779
7e74650
04d25fc
df0f09d
3184e93
72dbdf3
fe3c81a
0c925d9
8ff2eba
c9a2376
a63999c
6a273c1
f621766
690368d
892fc28
a1b492c
9f3480c
9eabbc0
5da8c11
46b7025
10336b0
8d09bac
d82eaea
a37cd7f
6b1c226
d02197e
99fb271
49d0155
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,11 +18,12 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle( | |
description, | ||
toolbar: { icon: _icon, items, title: _title, preventDynamicIcon, dynamicTitle }, | ||
}) => { | ||
const [globals, updateGlobals] = useGlobals(); | ||
const [globals, updateGlobals, storyGlobals] = useGlobals(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the API that addons use to tell if the user has overridden a global value they are interested in. If If the addon wants to know the underlying |
||
const [isTooltipVisible, setIsTooltipVisible] = useState(false); | ||
|
||
const currentValue = globals[id]; | ||
const hasGlobalValue = !!currentValue; | ||
const isOverridden = id in storyGlobals; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you mean:
But that check won't work because in the typical case the field will be defined and equal in both places. So you'd want:
But technically that could be wrong too -- you might override the (story) global to the same as the current (user) value -- in which case the toolbar should still be disabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the conclusion on this topic? |
||
let icon = _icon; | ||
let title = _title; | ||
|
||
|
@@ -42,7 +43,7 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle( | |
(value: string | undefined) => { | ||
updateGlobals({ [id]: value }); | ||
}, | ||
[currentValue, updateGlobals] | ||
[id, updateGlobals] | ||
); | ||
|
||
return ( | ||
|
@@ -79,6 +80,7 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle( | |
> | ||
<ToolbarMenuButton | ||
active={isTooltipVisible || hasGlobalValue} | ||
disabled={isOverridden} | ||
ndelangen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description={description || ''} | ||
icon={icon} | ||
title={title || ''} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,3 +31,9 @@ export default { | |
}; | ||
|
||
export const Basic = {}; | ||
|
||
export const Override = { | ||
globals: { | ||
locale: 'kr', | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmeasday Can I get an explainer why
defaultValue: 'fooDefaultValue'
should ever be used?How are
initialGlobals.foo
andglobalTypes.foo.defaultValue
connected?At first glance, me and @valentinpalkovic both concluded that if you set a
initialGlobal.foo
, thenglobalTypes.foo.defaultValue
should never be used?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh pray tell? I don't recall but it does seem to be that they are synonyms for each other (and we should probably rename
.defaultValue
also.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just a loose end/oversight. Originally we designed args and globals side by side. Both schemas had a defaultValue. We removed
argTypes.defaultValue
but never removedglobalTypes.defaultValue
. We probably should figure out what to do in this project.