Skip to content

Conversation

@smn
Copy link
Contributor

@smn smn commented May 21, 2015

No description provided.

@Rizziepit
Copy link
Contributor

Is locale negotiation being configured like here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this could be a custom Jinja2 filter instead of a view method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should only use the locale codes in view logic and leave it to the template to convert it to a display name

@Rizziepit
Copy link
Contributor

@nathanbegbie sorry, I might have started reviewing this early. Let me know if/when I should re-review

@nathanbegbie
Copy link
Contributor Author

@Riz he he, no worries. It doesn't actually work yet, still trying to get some tests passing. Will give you a shout when it's more better

@smn
Copy link
Contributor

smn commented May 22, 2015

Agreed on some of this stuff needing to be custom filters or simply added to the default template context in the context() function.

@nathanbegbie
Copy link
Contributor Author

@smn @miltontony @Riz Please review

@miltontony
Copy link
Contributor

👍 once travis passes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can now be display_languages because it's a variable in the default template context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. The problem I was trying to fix was that the ordering of the languages is not defined, so the selected language is not displayed first. But my change seems to break many things /o\ . I think I'm going to see if I can sort this out in the template

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 think we can specify the ordering in the view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, ordering would become easier if this is just a list of language codes.

@nathanbegbie
Copy link
Contributor Author

@Rizziepit @miltontony @smn OK, this time for sure, +1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably want to skip lang_code here if it's the same as language because otherwise we'd potentially get it twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's taken care of here https://github.com/universalcore/springboard/blob/feature/issue-21-add-locale-change/springboard/views.py#L55 in order to avoid duplication, but I'll put a check in the template, just to be sure

@smn
Copy link
Contributor

smn commented May 22, 2015

👍 nice work. Please land after the template change to check if the language == lang_code.

@smn
Copy link
Contributor

smn commented May 23, 2015

👍 hadn't seen the filtering being applied in the views.

@nathanbegbie nathanbegbie merged commit bd6a6b8 into develop May 24, 2015
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.

5 participants