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 schema for Dolphiq/craft3-iconpicker #193

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

u12206050
Copy link

No description provided.

@markhuot
Copy link
Owner

This is cool! The enum values look a little odd to me, though. I would recommend taking the GraphQL approach of upper casing them, like: as:HEX or a Twig approach of camel casing them, like: as:hex.

Other than that this is good to merge.

Thoughts?

@u12206050
Copy link
Author

Good point, however I feel since I want to keep it inline with how the original plugin uses it in templating dropping the "icon" part. So instead of iconCharHex, just CharHex

@markhuot
Copy link
Owner

I'll vote once again to lcfirst it, though. If the the icon field's Twig implementation dropped the icon prefix it would probably be written as, {{ entry.iconField.charHex }} since Craft prefers camel case for variable names.

@u12206050
Copy link
Author

Agree, will change to lcfirst

@u12206050
Copy link
Author

Changes made. Also, changed default to Char since that is most likely what I believe users will want

@u12206050
Copy link
Author

Ready to merge?

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