-
-
Notifications
You must be signed in to change notification settings - Fork 172
feat: add vue icons #294
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
feat: add vue icons #294
Conversation
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.
Looks great, thank you for getting the ball rolling on this!
I am fine with the props being kebab-case instead of camelCase, as long as it's consistent with what Vue users are expecting. So if Vue users expect them to be kebab-case, let's leave them that way. If they're expecting camelCase, let's update them to be camel case.
On the exports field, I'm of the opinion that the best approach for now would be to preserve support for TS, and then we can address the exports issue at the same time we re-approach it for the React packages (once TS decides to be a little friendlier).
So I would setup the exports to mirror the React packages for now (with support for ESM packages, etc), and then we can address the individual package export issues later, as part of the issue you mentioned. It's also important to make sure we still support both ESM + CommonJS, just like the React packages do.
I've reviewed the build script, and that all looks great as well.
README.md
Outdated
@@ -53,6 +53,10 @@ A React library is available to install under the name `iconoir-react`. For more | |||
|
|||
A React Native library is available to install under the name `iconoir-react-native`. For more details, see the package [README](./packages/iconoir-react-native). | |||
|
|||
## Vue | |||
|
|||
A Vue library is available to install under the name `iconoir-vue`. For more details, see the package [README](./packages/iconoir-vue). |
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.
@lucaburgio / @paescuj should confirm but I believe we decided whenever any new packages are to be released, we'll have them named with an organization prefix.
Can you rename any mention of iconoir-vue
with @iconoir/vue
instead?
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.
Oh yes, please 😍
All issues except the prop casing should be fixed now. While I'd guess that camelCase is a bit more idiomatic, SVG has a mix of camelCase and kebab-case attributes. Therefore, we'd need to manually transform the kebab-case attributes from camelCase back to kebab-case, which feels a little cumbersome: interface Props {
iconProps: CamelCaseSVGAttributes;
}
const props = defineProps<Props>();
const kebabCaseProps = {
"accentHeight": "accent-height",
"alignmentBaseline": "alignment-baseline",
"arabicForm": "arabic-form",
...
};
const transformedProps = computed(() => {
const kebabCaseKeys = Object.keys(kebabCaseProps);
const result = {};
for(const key of Object.keys(props.iconProps)) {
if (kebabCaseKeys.includes(key)) {
result[kebabCaseProps[key]] = props.iconProps[key];
} else {
result[key] = props.iconProps[key];
}
}
return result;
});
provide(providerKey, transformedProps); Do you have any better ideas? |
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.
I would say we just leave the props as-is, that way it's similar to what you would be expecting if you're using SVG inside Vue.
It all looks good to me! I will merge this at some point this week and get this automatically publishing to NPM, and then will post back here when it's done!
Thank you for the help on this one!
Closes #124
Adds Vue components for each icon and a
<IconoirProvider :icon-props="{...}" />
to match the features of the React package.It also adds support for both Vue 2 and Vue 3, thanks to the
vue-demi
package.I've still got a couple of issues:
IconoirProvider prop names
The
iconProps
prop uses theSVGAttributes
type, which doesn't use camelCase. This leads to the following code:Is this okay or would you prefer camelCase props?
Build file structure
Currently, the
buildVue.js
file is located in/bin
, which may not be the most optimal location. Any suggestions on how to structure this?Package.json exports
I added the
"./*"
exports field to the package.json file to allow for individual exports of icons. However, it seems that Typescript doesn't work well with this format, as mentioned in issue #243. Should we still keep this field?This is my first feature PR so any feedback is greatly appreciated!