Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Whitelist for SVG style attributes on data objects #333

Merged
merged 4 commits into from
Jan 25, 2018

Conversation

kale-stew
Copy link

sanitizeStyleProps is running all user-input style data against a whitelist of valid SVG attributes and returning an object that contains only valid styles, dropping the rest.

Should resolve Victory Issue #610

@kale-stew kale-stew requested a review from boygirl January 24, 2018 23:25
@boygirl
Copy link
Contributor

boygirl commented Jan 24, 2018

@kale-stew this is looking good. My only nitpick is that I'd like your sanitizeStyle method to live in the style util alongside the whitelist. Approved pending that change.

@kale-stew
Copy link
Author

@boygirl done 👍

* @param {Object} data An object of user-input style attributes.
* @returns {Object} An object containing only valid style data.
*/
export const sanitizeStyleProps = function (data) {
Copy link
Contributor

@boygirl boygirl Jan 25, 2018

Choose a reason for hiding this comment

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

I think this export is unnecessary, since you're exporting this method with your export default below

Copy link
Contributor

Choose a reason for hiding this comment

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

I just want to add a quick note here - I was pairing with Kylie and noticed that we cannot use named imports from default exports. I'm unfamiliar with the project semantics, but is this intentional?

ie, removing export here but then using the named import { sanitizeStyleProps } from './style' will cause sanitizeStyleProps to be undefined.

It looks the the way to avoid that is to import the entire module and path to the method like this

import Style from "../victory-util/style";

Not sure if this is a bug, or intentional but wanted to alert you, or @ryan-roemer to it.

@boygirl
Copy link
Contributor

boygirl commented Jan 25, 2018

🎉 Looks good!

@boygirl boygirl merged commit ad95542 into master Jan 25, 2018
@boygirl boygirl deleted the feature/610-style-whitelist branch January 25, 2018 18:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitelist style attributes on data objects
3 participants