-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Lodash: Refactor components away from _.find()
#46537
Conversation
@@ -290,8 +289,7 @@ function useAutocomplete( { | |||
const textAfterSelection = getTextContent( | |||
slice( record, undefined, getTextContent( record ).length ) | |||
); | |||
const completer = find( | |||
completers, | |||
const completer = completers?.find( |
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.
Relies on the editor.Autocomplete.completers
filter, so although we have completers enabled by default, it could end up being nullish. Adding optional chaining just in case.
@@ -23,7 +18,7 @@ export default function BottomSheetPickerCell( props ) { | |||
onChangeValue( newValue ); | |||
}; | |||
|
|||
const option = find( options, { value } ); | |||
const option = options.find( ( opt ) => opt.value === value ); |
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.
options
will default to an array:
options = [], |
const mappedColor = find( defaultColors, { | ||
slug: value, | ||
} ); | ||
const mappedColor = Object.values( defaultColors ?? {} ).find( |
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.
This comes in through an externally exposed API, so we're properly handling objects and nullish values.
@@ -143,6 +143,7 @@ export function getBlockTypography( | |||
const typographyStyles = {}; | |||
const customBlockStyles = blockStyleAttributes?.style?.typography || {}; | |||
const blockGlobalStyles = baseGlobalStyles?.blocks?.[ blockName ]; | |||
const parsedFontSizes = Object.values( fontSizes ?? {} ); |
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.
This comes in through an externally exposed API, so we're properly handling objects and nullish values.
It's used twice below, which is why we've moved it here.
const mappedFontSize = find( fontSizes, { | ||
slug: fontSize, | ||
} ); | ||
const mappedFontSize = parsedFontSizes.find( |
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.
See above comment on L146.
const mappedFontSize = find( fontSizes, { | ||
slug: blockStyleAttributes?.fontSize, | ||
} ); | ||
const mappedFontSize = parsedFontSizes.find( |
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.
See above comment on L146.
const matchedValue = find( mappedPresetValue.values, { | ||
slug: path[ 1 ], | ||
} ); | ||
const matchedValue = Object.values( |
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.
Will be an object:
gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js
Lines 287 to 296 in 47a6e4a
const mappedValues = { | |
color: { | |
values: colors, | |
slug: 'color', | |
}, | |
'font-size': { | |
values: fontSizes, | |
slug: 'size', | |
}, | |
}; |
The ?? {}
adds additional safety in the case of non-existence.
const matchedValue = find( mappedValues.color?.values, { | ||
slug: $2, | ||
} ); | ||
const matchedValue = mappedValues.color?.values?.find( |
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.
Same as above.
@@ -102,7 +101,7 @@ export function TabPanel( { | |||
) => { | |||
child.click(); | |||
}; | |||
const selectedTab = find( tabs, { name: selected } ); | |||
const selectedTab = tabs.find( ( { name } ) => name === selected ); |
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.
tabs
is being used as an array universally in this component already.
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.
Thanks for the detailed comments, @tyxla!
The code looks good, and I couldn't spot any regressions in web versions. I don't have a setup for the mobile app but looks safe to merge.
Size Change: +19 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
What?
This PR removes Lodash's
_.find()
from thecomponents
package. There are a few usages in that package and conversion is pretty straightforward.Why?
Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:
@wordpress/api-fetch
package haslodash
as a dependency #39495How?
We're using
Array.prototype.find()
instead of_.find()
. Use optional chaining and conversion to array with a fallback when necessary.Testing Instructions
/
in a new paragraph block).[[
in a new paragraph block).