refactor(core): refactor SettingsStore and useStructureToolSetting #5784
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
No changes to documentation |
|
Component Testing Report Updated Feb 23, 2024 3:10 PM (UTC)
|
|
|
||
| const set = useCallback( | ||
| (newValue: ValueType) => { | ||
| setValue(newValue) | ||
| settings.set(newValue as any) | ||
| KVStore.setKey(KVStoreKey, newValue as string) |
There was a problem hiding this comment.
Per my above comment about the JSON typings, this is being cast a string since that's technically what is always getting stored (for now). Should we export the KVStoreValue type so it can be used by all consumers?
| () => settingsStore.forNamespace('structure-tool'), | ||
| [settingsStore], | ||
| ) | ||
| const KVStoreKey = namespace ? `structure-tool::${namespace}::${key}` : `structure-tool::${key}` |
There was a problem hiding this comment.
We're having internal conversations about namespacing, so I don't expect this string interpolation to be permanent, hopefully
cngonzalez
force-pushed
the
sdx-1098
branch
2 times, most recently
from
69dfd13
to
3c4b9cb
Compare
|
Updates per feedback: |
| @@ -27,7 +27,6 @@ export function InspectDialog(props: InspectDialogProps) { | |||
| where the inspect dialog lives. | |||
| This also means that when a page is loaded, the state of the tabs remains and doesn't revert to the pane tab */ | |||
| const [viewModeId, onViewModeChange] = useStructureToolSetting( | |||
| 'structure-tool', | |||
There was a problem hiding this comment.
Don't have full context if it's used in multiple places, but the function signature has not changed, won't inspect-view-preferred-view-mode-${paneKey} become the namespace here? Is that expected?
There was a problem hiding this comment.
Good point -- so it looks like the behavior we have in the current version of the studio makes a key like structure-tool::structure-tool::inspect-view (useStructureToolSetting always added the structure-tool namespace). It seems like this was an error.
I can restore this to have the old duplicative behavior to not break anything and we can migrate it when we have a strategy to migrate all old keys.
Description
As we move toward different possible solutions for storing user values, and storing additional types of values, we can align our current implementations towards our proposed APIs. This PR is intentionally lightweight:
SettingsStoretoKVStoreto better reflect the additional kinds of values that may be stored.useStructureToolSettingto align with the above.What to review
The changed files to ensure they're aligned with our style.
Testing
Automated tests were introduced in #5764.
If needed, reviewer can confirm that localStorage and UI behavior remains undisturbed.