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

feat: Event Schema.org node #10

Merged
merged 7 commits into from
Aug 3, 2022
Merged

feat: Event Schema.org node #10

merged 7 commits into from
Aug 3, 2022

Conversation

cyruscollier
Copy link
Contributor

No description provided.

@cyruscollier cyruscollier marked this pull request as ready for review July 7, 2022 16:01
@harlan-zw
Copy link
Collaborator

Hey @cyruscollier

Thanks so much for this, it's an amazing PR with a lot of thought and effort put into it.

Starting with the changes in types.ts I think you may have missed the direction I wanted this package to have, though I can definitely appreciate why you've done it this way.

I purposely avoided implementing types from schema-dts. I've discussed why I went this route in this comment and the docs

Mainly I wanted to provide a simple API for end-users, only typing fields that Google has documented will affect Rich Results. In my opinion, the setup you have isn't a very nice DX and adds maintenance debt keeping things in sync with schema-dts.

End users who want to implement full typescript support on schema can make use of custom schema. Though the schema-dts usage still needs to be documented.

So to move forward with this PR, I'd like to see:

From there it would be a lot easier to provide more granular feedback.

Again I truly appreciate the effort in this PR and if you disagree with anything I've said, please let me know.

…red Data, removed CreativeWork and Place root nodes
@cyruscollier
Copy link
Contributor Author

Thanks @harlan-zw , your feedback makes a lot of sense, especially about keeping the DX simple and only implementing what's supported by Google Structured Data. I'm actually a little relieved because I felt like I was starting to go down a rabbit hole trying to flush out the full Schema.org specs!

I've removed CreativeWork and Place as root nodes, and now Place is just a simple dependent interface as part of Event, although it may need to be moved into the shared types file if other nodes need it in the future.

@harlan-zw harlan-zw changed the title Event, CreativeWork and Place Nodes feat: Event Schema.org node Jul 11, 2022
@cyruscollier
Copy link
Contributor Author

Hey @harlan-zw , any feedback on this revised PR? Thanks!

@harlan-zw
Copy link
Collaborator

Hey @cyruscollier

Thanks for the update to the PR! Sorry I missed this notification somehow, I'll give it a look this weekend and try get it merged!

@harlan-zw harlan-zw merged commit 4216cea into vueuse:main Aug 3, 2022
@harlan-zw
Copy link
Collaborator

Thanks again @cyruscollier. I'm working on a major upgrade to this package and moving the node logic to https://github.com/harlan-zw/schema-org-graph

I'll migrate these changes, they'll be available in a beta that I'll push out today hopefully

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.

None yet

2 participants