-
Notifications
You must be signed in to change notification settings - Fork 525
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
Added functionality to be able to pass user props to components. Safe… #2151
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import React from "react"; | ||
import { VictoryStack } from "@packages/victory-stack/src/index"; | ||
import { VictoryArea } from "@packages/victory-area/src/index"; | ||
|
||
class App extends React.Component { | ||
|
||
render() { | ||
return ( | ||
<div> | ||
<h3 style={{textAlign: 'center'}}>Standalone Stack</h3> | ||
<VictoryStack | ||
aria-label="Victory Stack Demo" | ||
data-some-user-prop="TESTING 123" | ||
> | ||
<VictoryArea | ||
data={[{x: "a", y: 2}, {x: "b", y: 3}, {x: "c", y: 5}]} | ||
/> | ||
<VictoryArea | ||
data={[{x: "a", y: 1}, {x: "b", y: 4}, {x: "c", y: 5}]} | ||
/> | ||
<VictoryArea | ||
data={[{x: "a", y: 3}, {x: "b", y: 2}, {x: "c", y: 6}]} | ||
/> | ||
</VictoryStack> | ||
</div> | ||
) | ||
} | ||
} | ||
|
||
export default App; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/* | ||
USER_PROPS_SAFELIST is to contain any string deemed safe for user props. | ||
The startsWidth array will contain the start of any accepted user-prop that | ||
starts with these characters. | ||
The exactMatch will contain a list of exact prop names that are accepted. | ||
*/ | ||
const USER_PROPS_SAFELIST = { | ||
startsWith: ["data-", "aria-"], | ||
exactMatch: [] | ||
}; | ||
|
||
/** | ||
* doesPropStartWith: Function that takes a prop's key and runs it against all | ||
* options in the USER_PROPS_SAFELIST and checks to see if it starts with any | ||
* of those options. | ||
* @param {string} key: prop key to be tested against whitelist | ||
* @returns {Boolean}: returns true if the key starts with an option or false if | ||
* otherwise | ||
*/ | ||
const doesPropStartWith = (key) => { | ||
let startsWith = false; | ||
|
||
USER_PROPS_SAFELIST.startsWith.forEach((starterString) => { | ||
const regex = new RegExp(`\\b(${starterString})(\\w|-)+`, "g"); | ||
if (regex.test(key)) startsWith = true; | ||
}); | ||
|
||
return startsWith; | ||
}; | ||
|
||
/** | ||
* isExactMatch: checks to see if the given key matches any of the 'exactMatch' | ||
* items in the whitelist | ||
* @param {String} key: prop key to be tested against the whitelist-exact match | ||
* array. | ||
* @returns {Boolean}: return true if whitelist contains that key, otherwise | ||
* returns false. | ||
*/ | ||
const isExactMatch = (key) => USER_PROPS_SAFELIST.exactMatch.includes(key); | ||
|
||
/** | ||
* testIfSafeProp: tests prop's key against both startsWith and exactMatch values | ||
* @param {String} key: prop key to be tested against the whitelist | ||
* @returns {Boolean}: returns true if found in whitelist, otherwise returns false | ||
*/ | ||
const testIfSafeProp = (key) => { | ||
if (doesPropStartWith(key) || isExactMatch(key)) return true; | ||
return false; | ||
}; | ||
|
||
/** | ||
* getSafeUserProps - function that takes in a props object and removes any | ||
* key-value entries that do not match filter strings in the USER_PROPS_SAFELIST | ||
* object. | ||
* | ||
* @param {Object} props: props to be filtered against USER_PROPS_SAFELIST | ||
* @returns {Object}: object containing remaining acceptable props | ||
*/ | ||
export const getSafeUserProps = (props) => { | ||
const propsToFilter = { ...props }; | ||
return Object.fromEntries( | ||
Object.entries(propsToFilter).filter(([key]) => testIfSafeProp(key)) | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,13 @@ import { assign, defaults, isEmpty } from "lodash"; | |
import PropTypes from "prop-types"; | ||
import React from "react"; | ||
import { | ||
CommonProps, | ||
Helpers, | ||
Hooks, | ||
UserProps, | ||
VictoryContainer, | ||
VictoryTheme, | ||
CommonProps, | ||
Wrapper, | ||
Hooks | ||
} from "victory-core"; | ||
import { VictorySharedEvents } from "victory-shared-events"; | ||
import { getChildren, useMemoizedProps } from "./helper-methods"; | ||
|
@@ -88,17 +89,21 @@ const VictoryGroup = (initialProps) => { | |
name | ||
]); | ||
|
||
const userProps = React.useMemo(() => UserProps.getSafeUserProps(initialProps), [initialProps]); | ||
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'm not sure whether this For now, I wouldn't stress too much about memoizing this value since it isn't a very expensive function call. Unless you can see a performance difference, let's just remove the |
||
|
||
const container = React.useMemo(() => { | ||
if (standalone) { | ||
const defaultContainerProps = defaults( | ||
{}, | ||
containerComponent.props, | ||
containerProps | ||
containerProps, | ||
userProps | ||
); | ||
return React.cloneElement(containerComponent, defaultContainerProps); | ||
} | ||
return groupComponent; | ||
}, [groupComponent, standalone, containerComponent, containerProps]); | ||
|
||
return React.cloneElement(groupComponent, userProps); | ||
}, [groupComponent, standalone, containerComponent, containerProps, userProps]); | ||
|
||
const events = React.useMemo(() => { | ||
return Wrapper.getAllEvents(props); | ||
|
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.
Where is
parentProps.userProps
coming from? Is this outdated, or are we still setting this somewhere in agetComponentProps
function?