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

Add content to MetaTagsNotification objects #194

Conversation

ianleeder
Copy link
Contributor

Fixes #193

Ideally I would have removed the original (string contentTypeAlias, MetaTagsModel metaTags) constructors, but I kept them for strict backwards compatibility. I struggle to imagine a scenario where end-users are creating these classes themselves, but they are public, and it is possible.

Again, this was based dev/Umbraco10

@patrickdemooij9
Copy link
Owner

Hi @ianleeder,
Thanks for this PR as well! Seems like a logical choice. Will merge it straight away

@patrickdemooij9 patrickdemooij9 merged commit bb154fb into patrickdemooij9:dev/Umbraco10 May 31, 2023
@@ -7,11 +9,20 @@ public class AfterMetaTagsNotification : INotification
{
public string ContentTypeAlias { get; }
public MetaTagsModel MetaTags { get; }
public IPublishedContent Content { get; set; }
Copy link
Contributor Author

@ianleeder ianleeder Jun 1, 2023

Choose a reason for hiding this comment

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

@patrickdemooij9
I just saw that I allowed set on the automatic property. This should probably be updated to get only, like the other properties. Just habit (and auto-complete).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR #199 to fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I don't use GitHub PRs very often. I couldn't see a way to amend or reopen this one once it was merged. Let me know if there was an easier way to do this. A second PR feels like overkill)

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