feat: Persist SQL Lab autocomplete setting across tabs and visits #17708
Conversation
etr2460
force-pushed
the
erik-ritter--persist-sql-autocomplete-setting
branch
2 times, most recently
from
489bdb9
to
e1c1d19
Compare
etr2460
force-pushed
the
erik-ritter--persist-sql-autocomplete-setting
branch
from
e1c1d19
to
60f2ef6
Compare
etr2460
force-pushed
the
erik-ritter--persist-sql-autocomplete-setting
branch
2 times, most recently
from
cfad894
to
6580525
Compare
| if (value === null) { | ||
| return defaultValue; | ||
| } | ||
| return JSON.parse(value); |
There was a problem hiding this comment.
this was broken in a very subtle way before. If the value in localStorage was falsey (0, false, '', etc.), then this function would return the defaultValue.
unfortunately, localStorage doesn't have a hasItem function, so the best thing we can do is see if getItem returns null (https://developer.mozilla.org/en-US/docs/Web/API/Storage/getItem#return_value). Isn't JS fun!
|
/testenv up |
|
@etr2460 Ephemeral environment spinning up at http://34.219.206.85:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #17708 +/- ##
==========================================
- Coverage 67.16% 67.16% -0.01%
==========================================
Files 1608 1608
Lines 64798 64801 +3
Branches 6851 6851
==========================================
+ Hits 43520 43521 +1
- Misses 19419 19421 +2
Partials 1859 1859
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -65,3 +65,6 @@ export const SQL_FUNCTIONS_AUTOCOMPLETE_SCORE = 90; | |||
| export const SCHEMA_AUTOCOMPLETE_SCORE = 60; | |||
| export const TABLE_AUTOCOMPLETE_SCORE = 55; | |||
| export const COLUMN_AUTOCOMPLETE_SCORE = 50; | |||
|
|
|||
| // LocalStorage keys | |||
| export const SQL_LAB_AUTOCOMPLETE_SETTING_KEY = 'isSQLLabAutocompleteEnabled'; | |||
There was a problem hiding this comment.
Should this be SQL_LAB_AUTOCOMPLETE_ENABLED_SETTING_KEY?
| SQL_LAB_AUTOCOMPLETE_SETTING_KEY, | ||
| !prevState.autocompleteEnabled, | ||
| ); | ||
| return { |
There was a problem hiding this comment.
Why is a return necessary when it wasn’t previously?
There was a problem hiding this comment.
It does previously. An arrow function with just expression will use the value of that expression as the return value.
| @@ -65,3 +65,6 @@ export const SQL_FUNCTIONS_AUTOCOMPLETE_SCORE = 90; | |||
| export const SCHEMA_AUTOCOMPLETE_SCORE = 60; | |||
| export const TABLE_AUTOCOMPLETE_SCORE = 55; | |||
| export const COLUMN_AUTOCOMPLETE_SCORE = 50; | |||
|
|
|||
| // LocalStorage keys | |||
| export const SQL_LAB_AUTOCOMPLETE_SETTING_KEY = 'isSQLLabAutocompleteEnabled'; | |||
There was a problem hiding this comment.
Would it be better if we keep all local storage keys as an enum and move into localStorageHelpers.js? The keys are global so it makes sense to keep them in a centralized place.
The values can even be typed:
/**
* Local storage manager
*/
export function safeJsonParse(jsonString: string | null) {
if (jsonString === null) return null;
try {
return JSON.parse(jsonString);
} catch {
return null;
}
}
export type StorageValues = {
key1: Type1,
key2: Type2,
};
export type StorageKey = keyof StorageValues;
export const defaultStorageValues: StorageValues = {
key1: value1,
key2: value2
};
export function entries() {
Object.values(Storage).forEach(key => {
return [key, getItem(key)];
});
}
export function getItem<K extends StorageKey>(key: K): StorageValues[K] {
return safeJsonParse(localStorage.getItem(key)) ?? defaultStorageValues[key];
}
export function setItem(key: StorageKey, value: string | number | object) {
return localStorage.setItem(key, JSON.stringify(value));
}There was a problem hiding this comment.
yeah, that was my original thought, but i realized the existing keys that are used don't do that yet. maybe i'll make a PR first to centralize all the keys, then i'll rebase this one on it
There was a problem hiding this comment.
I'm going to implement this now, but probably won't add default values here, if only because i think sometimes the caller might want to se a different default
|
will rebase on #17832 before merging |
etr2460
force-pushed
the
erik-ritter--persist-sql-autocomplete-setting
branch
from
6580525
to
24c3d9a
Compare
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
Follows the pattern used for the filter box migration and the welcome page settings to make the SQL Lab autocomplete setting persistent across SQL Lab tabs and Superset sessions/visits
TESTING INSTRUCTIONS
Go to SQL Lab, see autocomplete is on by default. If you disable it and switch tabs, see it disabled. If you reload the page, see it still disabled. Reenable it, reload the page, and see it still enabled
ADDITIONAL INFORMATION
to: @michael-s-molina @graceguo-supercat @ktmud @rusackas