feat(feature-toggles): Add feature object to standardize feature toggles #1477
Conversation
src/lib/Preview.js
Outdated
| // Features | ||
| // This makes features available everywhere that features is passed in, which includes | ||
| // all of the viewers for different files | ||
| this.options.features = options.features; |
There was a problem hiding this comment.
Can we move this before the enable/disable viewer logic? Should it have a default value, such as an empty object?
There was a problem hiding this comment.
Good point, will do
src/lib/__tests__/Preview-test.js
Outdated
| @@ -38,6 +38,8 @@ let preview; | |||
| let containerEl; | |||
|
|
|||
| describe('lib/Preview', () => { | |||
| const features = { shouldUseBatman: { enabled: true } }; | |||
There was a problem hiding this comment.
Probably best to avoid copyrighted characters. 😄
| @@ -0,0 +1,34 @@ | |||
| import { getFeatureConfig, isFeatureEnabled } from '../featureChecking'; | |||
There was a problem hiding this comment.
Can this be converted to a TS file, as well?
| @@ -0,0 +1,34 @@ | |||
| import { getFeatureConfig, isFeatureEnabled } from '../featureChecking'; | |||
|
|
|||
| const Box = { | |||
There was a problem hiding this comment.
Can we remove the Box wrapper?
src/lib/featureChecking.ts
Outdated
| [key: string]: FeatureOptions; | ||
| }; | ||
|
|
||
| const isFeatureEnabled = (features: FeatureConfig, featureName: string): boolean => { |
There was a problem hiding this comment.
If we still want a singleton style implementation rather than passing the value around, something like the following would work:
let features = {};
const setFeatures = value => {
features = value || {};
}
const isFeatureEnabled = (featureName: string): boolean => {
return !!get(features, featureName, false);
}
// Preview.js
features.setFeatures(options.features);
src/lib/featureChecking.ts
Outdated
| return get(features, featureName, {}); | ||
| }; | ||
|
|
||
| export { isFeatureEnabled, getFeatureConfig }; |
There was a problem hiding this comment.
Can we export inline?
export const getFeatureConfig ...
| }, | ||
| }; | ||
|
|
||
| describe('isFeatureEnabled', () => { |
There was a problem hiding this comment.
Minor, but we typically maintain one top-level describe per test file. Can we wrap these two?
src/lib/constants.js
Outdated
| @@ -102,7 +102,7 @@ export const PDFJS_WIDTH_PADDING_PX = 40; // Should match SCROLLBAR_PADDING in p | |||
| export const PDFJS_HEIGHT_PADDING_PX = 5; // Should match VERTICAL_PADDING in pdf_viewer.js | |||
|
|
|||
| export const DOC_LEGACY_STATIC_ASSETS_VERSION = '2.76.0'; // IE11 only | |||
| export const DOC_STATIC_ASSETS_VERSION = '2.93.0'; // Modern browsers | |||
| export const DOC_STATIC_ASSETS_VERSION = '2.90.0'; // Modern browsers | |||
There was a problem hiding this comment.
Can we make this change as a separate PR?
Purpose is to enhance the Preview SDK to standardize the usage of feature toggles. This takes in a features object passed in from the options object and sets to the Preview options field. This allows Preview items to have access to the features passed in from the Preview SDK caller.