Skip to content
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

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

dlcartright
Copy link
Contributor

Description:

Passed missing 'title' and 'desc' props to VictoryContainer component from VictoryChart component. Added functionality for users to pass other attributes to the container SVG that pass the whitelist test (defined in victory/packages/victory-core/src/victory-util/user-props.js). Safe-list currently allows props starting with 'aria-' and 'data-'. VictoryChart, VictoryGroup, VictoryStack, and VictoryContainer have all been wired up to allow these user props. More components to follow.

Currently the whitelist contains 2 arrays called "startsWith" and "exactMatch". Any props will be run against these two to see if they start with any of the items in the startsWith array or they match exactly with items from the exactMatch array.

Testing:

Start this branch with yarn start and navigate to the Chart example.

Test 1

  1. Inspect the first chart in DevTools, you will see an attribute added to the SVG called "data-some-user-prop" as well as an "aria-label".
  2. Any of other attributes passed into VictoryChart (that are allowed in the whitelist) will be added here as well. So, if you wish, add additional attributes to the example and inspect to see that they carried over.
    UserAttributes

Test 2

  1. Using the browser extension called Accessibility Insights for Web run a Fast Pass on the chart example page.
  2. The first 4 charts will be accessible (because they have a title attribute added to them). The others will be outlined in red and show the error below. It is up to the user to add the title to their charts.

ChartsMissingTitle

@dlcartright dlcartright self-assigned this Mar 5, 2022
@dlcartright
Copy link
Contributor Author

Will be making additional PRs into this branch for the remaining components to break up the work and keep the PRs manageable.

…list currently allows props starting with 'aria-' and 'data-'. VictoryChart, VictoryGroup, VictoryStack, and VictoryContainer have all been wired up to allow these user props. More components to follow.
@dlcartright dlcartright force-pushed the allow-user-props-foundation branch from ac15507 to c5ee839 Compare March 5, 2022 22:20
@dlcartright dlcartright marked this pull request as ready for review March 14, 2022 13:17
@@ -88,17 +89,21 @@ const VictoryGroup = (initialProps) => {
name
]);

const userProps = React.useMemo(() => UserProps.getSafeUserProps(initialProps), [initialProps]);
Copy link
Contributor

@becca-bailey becca-bailey Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this useMemo is doing any good here. Since initialProps isn't stored in a state or ref value, I believe useProps will be re-calculated on every render. There is a fairly good explanation here on how to compare objects in hooks. https://stackoverflow.com/questions/54095994/react-useeffect-comparing-objects

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 useMemo for now.

if (parentProps.events) {
this.globalEvents = Events.getGlobalEvents(parentProps.events);
parentProps.events = Events.omitGlobalEvents(parentProps.events);
}
return React.cloneElement(component, parentProps, children);

const componentProps = { ...parentProps, ...parentProps.userProps };
Copy link
Contributor

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 a getComponentProps function?

@becca-bailey
Copy link
Contributor

I have a couple questions and comments here, but let's merge this work into feature/user-props, and you (or anyone else who picks up the work) can address these comments in a follow-up PR to the same feature branch. I will also change your other PRs to merge into the feature branch, rather than into this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants