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

Fix for tinymce tests #853

Closed
wants to merge 1 commit into from
Closed

Fix for tinymce tests #853

wants to merge 1 commit into from

Conversation

lyralemos
Copy link
Contributor

This PR fix tinymce tests falling since #849.

@lyralemos lyralemos requested a review from agitator June 15, 2018 11:55
@petschki petschki mentioned this pull request Jun 22, 2018
Merged
petschki
petschki requested changes Jun 28, 2018
View reviewed changes
Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

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

OK, now I got a little deeper into this fix.
I've made some changes to this pull request #854 which fixes the tiny tests too (this is all due to broken JS loading from tinymce-builded/js/tinymce/langs/ folder)

But all in all I'm not sure if this - very TinyMCE specific - language code pattern should be the pattern of the i18n.currentLanguage. I'm more into creating a new var like tinymceLanguageCodewhich later can be used by the tinymce-pattern here:

mockup/mockup/patterns/tinymce/pattern.js

Line 292 in c84018d

var lang = i18n.currentLanguage;

Any thoughts on this @claytonc @thet @agitator ?

mockup/js/i18n.js
@@ -21,7 +21,7 @@ define([
self.currentLanguage = $('html').attr('lang') || 'en-us';
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend using the non country specific fallback language "en" and remove the extra check below
@lyralemos @thet @agitator @frapell your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me

@lyralemos
Copy link
Contributor Author

If the changes you mentioned in #854 fixes tinymce tests I see no reason why not to discard this PR.

What you said about i18n.currentLanguage being TinyMCE specific, actually the changes introduced in #849 fixes both tinyMCE and Folder Contents. This happens because all country specific languages codes in plone.app.locales are like pt_BR, for instance, and not like pt-br (which is the standard for html languages codes). For that reason I see no problem with transforming the language code.

@petschki
Copy link
Member

@lyralemos thanks for clarification. Then it's ok for me to transform the language code here. I wasn't aware of plone.app.locales country-language code pattern. So we actually can merge #854 and discard this PR.

@petschki
Copy link
Member

#854 with test fix is merged .... closing this

@petschki petschki closed this Jun 29, 2018
@davilima6
Copy link
Member

davilima6 commented Feb 16, 2019 •
edited
Loading

@lyralemos, I think the web standard is IETF ('pt-BR'):

The rules for creating language attribute values are described by an IETF specification called BCP 47. In addition to specifying how to use simple language tags, such as en for English or fr for French, BCP 47 describes how to compose language tags that allow you specify regional dialects, scripts and other variants related to that language.
https://www.w3.org/International/questions/qa-html-language-declarations

That's also the format you get when you visit one of Google or Mozilla websites so I suggest we should stick to it. And change our <html lang> formatting.

There's also POSIX ('pt_BR') to use for filenames, which TinyMCE does (but in a folder misnamed langs, since a locale is the sum of language and region). OTOH in Moment.js, folder is correctly named "locales" but files are IETF and lowercased. Maybe I miss something, if not looks like an inconsistency to me.

In any case it'd be useful to have attributes in mockup-i18n delivering each of these versions (not 100% sure about the lower case version, if it's an anti-pattern we shouldn't promote it). E.g.:

  • self.language: 'pt_BR' (POSIX) (as is today but renaming/aliasing/deprecating self.currentLanguage)
  • self.languageCode: 'pt' (POSIX without region code)
  • self.regionCode: 'BR' (POSIX without language code)
  • self.locale: 'pt-BR'
  • self.localeLower: 'pt-br' (I couldn't find a name for this pattern but Moment uses it for filenames - while preferring IETF in its API)

What do you think?
/cc @jensens @agitator @ale-rt @petschki @gforcada

I came to this because I didn't want to re-parse and treat defaults while manually getting to implement locale lazy loading for Moment.js. I preferred to delegate to mockup-i18n however after #849 I had to reformat the string which now comes as POSIX:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davilima6 davilima6 davilima6 left review comments

@petschki petschki petschki requested changes

@agitator agitator Awaiting requested review from agitator

Assignees
No one assigned
Labels
32 needs: review 99 tag: Plone 5.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lyralemos @petschki @davilima6