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.

Sign up for GitHub

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

Jump to bottom

[VUFIND-1653] Allow schema.org metadata to be disabled via config. #3634

Merged
merged 6 commits into from
May 14, 2024
Merged

[VUFIND-1653] Allow schema.org metadata to be disabled via config. #3634

merged 6 commits into from
May 14, 2024

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented May 1, 2024 •
edited

TODO

  • Add unit test for new helper once general design is approved
  • Update changelog when merging
  • Resolve or revise VUFIND-1653 when merging

@demiankatz demiankatz marked this pull request as draft May 1, 2024 16:57
@demiankatz demiankatz marked this pull request as ready for review May 1, 2024 19:24
@demiankatz demiankatz requested a review from EreMaijala May 1, 2024 19:24
@demiankatz demiankatz added this to the 10.0 milestone May 3, 2024
maccabeelevine
maccabeelevine approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@maccabeelevine maccabeelevine left a comment

Choose a reason for hiding this comment

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

Looks great, works great. I added two pedantic comments.

module/VuFind/src/VuFind/View/Helper/Root/RecordDataFormatterFactory.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/View/Helper/Root/SchemaOrg.php Show resolved Hide resolved
@demiankatz
Copy link
Member Author

Thanks, @maccabeelevine -- I've just addressed both of your review comments. Good catches/ideas! Now I just need to find time to improve test coverage before merging this. I'll try to do that very soon.

@demiankatz
Copy link
Member Author

Okay, I've added test coverage for the new helper; now I'll wait and give @EreMaijala a chance to review this before I merge, in case he has any final thoughts.

@demiankatz demiankatz merged commit 82d2415 into vufind-org:dev May 14, 2024
7 checks passed
@demiankatz demiankatz deleted the vufind-1653 branch May 14, 2024 15:09
@demiankatz demiankatz mentioned this pull request May 15, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EreMaijala EreMaijala EreMaijala approved these changes

@maccabeelevine maccabeelevine maccabeelevine approved these changes

Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
10.0
3 participants
@demiankatz @EreMaijala @maccabeelevine