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

Make sure to set the variationContext for sitemap #166

Merged
merged 3 commits into from
Mar 6, 2023
Merged

Make sure to set the variationContext for sitemap #166

merged 3 commits into from
Mar 6, 2023

Conversation

patrickdemooij9
Copy link
Owner

Fixes #162
And should also fix the issues reported by @ianleeder in #123

ianleeder
ianleeder reviewed Mar 6, 2023
View reviewed changes
Copy link
Contributor

@ianleeder ianleeder left a comment

Choose a reason for hiding this comment

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

Just as an FYI, it looks like you've committed these changes with a mixture of tabs/spaces. GitHub is showing tabs as 8 spaces so it's super-obvious in the web interface
image

...SeoToolkit.Umbraco.MetaFields.Core/Services/MetaFieldsValueService/MetaFieldsValueService.cs Outdated
}
}
ClearCache(nodeId);
}

public void Delete(int nodeId, string fieldAlias)
public void Delete(int nodeId, string fieldAlias, string culture)
Copy link
Contributor

Choose a reason for hiding this comment

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

@patrickdemooij9 Should this culture default to null, same as the interface?

Suggested change
public void Delete(int nodeId, string fieldAlias, string culture)
public void Delete(int nodeId, string fieldAlias, string culture = null)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good catch. Not quite sure why it wasn't complaining about it but I changed it to the default null now

@patrickdemooij9
Copy link
Owner Author

Thanks for the check on my code @ianleeder! Guess I still have to put some settings correct on my new laptop

@patrickdemooij9 patrickdemooij9 merged commit e2d4911 into dev/Umbraco10 Mar 6, 2023
@patrickdemooij9 patrickdemooij9 deleted the feature/CultureSitemap branch March 6, 2023 16:38
@ianleeder
Copy link
Contributor

Not a problem @patrickdemooij9. Any estimates on when you'll release the next version? My project is going live soon, and I'd like to slip this fix in if possible.

@patrickdemooij9 patrickdemooij9 mentioned this pull request Mar 7, 2023
Closed
@patrickdemooij9
Copy link
Owner Author

I am going to try and get this bug fixed: #164 and then I'll deploy a release. The new version will most likely be available before next saturday

patrickdemooij9 added a commit that referenced this pull request Mar 7, 2023
@patrickdemooij9
Make sure to set the variationContext for sitemap (#166)
b5bc982 
* Make sure to set the variationContext for sitemap

* Some fixes

* Last fix
@patrickdemooij9
Copy link
Owner Author

@ianleeder
The newest version is now out. This includes this change, so your code should work without issues now

@ianleeder ianleeder mentioned this pull request Nov 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianleeder ianleeder ianleeder left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@patrickdemooij9 @ianleeder