Skip to content

Conversation

@jonathoncove2
Copy link
Contributor

@jonathoncove2 jonathoncove2 commented Aug 19, 2024

Added some support for setting cultures in API calls

Description

What did you add/update/change?

Type of suggestion

  • Typo/grammar fix
  • Updated outdated content
  • New content
  • Updates related to a new version
  • Other

Product & version (if relevant)

Deadline (if relevant)

When should the content be published?

Added some support for setting cultures in API calls
@eshanrnh
Copy link
Contributor

Thanks for the PR, @jonathoncove2 🙌 We will review it as soon as we can. 💪

@sofietoft
Copy link
Contributor

Thanks for the PR @jonathoncove2 ! 💪

The "Creating a basic site" tutorial is meant to be very high-level and shouldn't dive into more code than the cshtml and CSS files.
Instead, a fit for this code of yours, could be in the "Creating a multilingual site" instead?
Let me know what you think.
I'll make sure we have the code reviewed in the meantime 💪

@jonathoncove2
Copy link
Contributor Author

Thanks for looking over this @sofietoft ! I agree that page is a more suitable place for this code. Would you prefer me to make a new PR for this?

I also considered some example code showing how to add a language switching menu, would that be a useful addition?

@sofietoft
Copy link
Contributor

Excellent!
No need for a new PR, I think. Just put the code in the other article instead, then we're golden ⭐

It would be amazing with a language switcher 👏
We've actually had that on an internal to-do list for ages, just never had the time to prioritize it.

The "Create a multilingual site" tutorial is a bit long and messy at the moment.
But if you could add your stuff where you think it makes sense, then I'll make sure we give it a look afterward - might need a bit of restructuring and perhaps it could even be split into multiple sub-articles 🤔

Added a guide for creating a basic language switching navigation
@jonathoncove2
Copy link
Contributor Author

I've removed that API example as requested and added a language switcher. Not sure if my link from adding-language-variants.md works correctly, that might need to be fixed. I've also added sub headers, hopefully to guide users googling the same things I was.

I'm about to go on holiday for a week and half, but feel free to Umbraco-ify the PR and approve it while I'm away.

@sofietoft
Copy link
Contributor

Amazing @jonathoncove2 !
I'll make sure we take a look at it as soon as possible 💪

Have a wonderful holiday 🏖️

Copy link
Contributor

@Frost117 Frost117 left a comment

Choose a reason for hiding this comment

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

@jonathoncove2 I made some comments about the ILocalizationService, do you think you can rewrite the part but using the ILanguage service instead?

```csharp
public class ExampleController : SurfaceController
{
private readonly ILocalizationService _localizationService;
Copy link
Contributor

Choose a reason for hiding this comment

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

The ILocalizationService is currently marked as obsolete and will be removed in v15. If we decide to keep this one, we have to include a note that the above is obsolete.

Preferably, make this example use the ILanguageService instead


public IActionResult Index(string culture = null)
{
IEnumerable<ILanguage> UmbracoLanguages = _localizationService.GetAllLanguages(); //a helpful method to get all configured languages
Copy link
Contributor

Choose a reason for hiding this comment

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


#Getting all the languages for a site

There are two ways to achive this. One is to use ```localizationService.GetAllLanguages();``` to call the database, which is expensive and ideally includes caching.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could still keep this part, but instead it would be 3 ways to get the languages.

Then show of each with the not that the localizationService very expensive to do and should only be done if you cannot use the ILanguage service

@sofietoft
Copy link
Contributor

Hello @jonathoncove2 👋

Did you notice the review on your PR here? 😄

…rvice

Updated the v10 docs to use the original example, as language service is not available
@jonathoncove2
Copy link
Contributor Author

@sofietoft so sorry about the late response to this.

I've updated the example as suggested, and added the original example to the v10 docs, as the original example is still the most appropriate for that version. I'm not sure when ILanguageService became available, so it might be best to add it to a lower version, you guys will know better than me on that one!

@sofietoft sofietoft requested a review from Frost117 October 15, 2024 08:25
@sofietoft
Copy link
Contributor

Perfect, thanks @jonathoncove2 ! 💪

I'll make sure our developer give this a second review.

Copy link
Contributor

@Frost117 Frost117 left a comment

Choose a reason for hiding this comment

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

Looks good with the v14 and the updated LanguageService 😃

Copy link
Contributor

@sofietoft sofietoft left a comment

Choose a reason for hiding this comment

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

I've taken the liberty to fix some grammar issues and some typos.
I've also fixed a couple of lists, where each item didn't start with the action word, which prefer 🙌

I was wondering if you could put the steps in the Steps section into an ordered list? Since they are steps, I think it would be nice to format them as such as well 😄

Finally, I've made a suggestion!
I hope it all makes sense. Let me know if something doesn't 💪


You need to replace `{{homeNodeContentAlias}}` with the Document Type alias of your Home node.

This will look at all the cultures available on the home node, and render links to either the language variant of the current page or the home node for the language variant. If the home node for a language variant is removed, it will not appear in the list.
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion to shorten this sentence a bit.

Suggested change
This will look at all the cultures available on the home node, and render links to either the language variant of the current page or the home node for the language variant. If the home node for a language variant is removed, it will not appear in the list.
This will look at all the cultures available on the home node. Links to either the language variant of the current page or the home node for the language variant will be rendered. If the home node for a language variant is removed, it will not appear in the list.

@jonathoncove2
Copy link
Contributor Author

Thanks Sophie! Looks like I rely on spell checkers too much 🙈

I've made the changes as you requested

@sofietoft
Copy link
Contributor

Looks great @jonathoncove2 ! 😄
Think for applying the suggestions.

I'lle get this merged as soon as the check clears 🙌

@sofietoft sofietoft merged commit e975476 into umbraco:main Oct 30, 2024
57 of 68 checks passed
@jonathoncove2
Copy link
Contributor Author

Thanks @sofietoft !

sofietoft added a commit that referenced this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants