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

Using safe string for slug attribute in navigation helper; Adding base64 slug #10267

Merged
merged 1 commit into from Dec 13, 2018

Conversation

TJYSunset
Copy link
Contributor

@TJYSunset TJYSunset commented Dec 10, 2018

Trying to resolve #10258.

Changed:

New:

  • Added a {{slug_base64}} attribute that generates utf-8 base64 of the navigation label.

Cultural invariant base64 is probably a better solution due to string.safe uses unidecode which provided a hilarious usage example:

> unidecode("に間違いがないか、再度確認してください。再読み込みしてください。");
'niJian Wei iganaika, Zai Du Que Ren sitekudasai. Zai Du miIp misitekudasai. '

The string in question should be romanized as nikanchigaiganaika, saidokakuninshitekudasai. saiyomikomishitekudasai. (well I admit algorithmic CJK romanization is hard.) But since that will break backwards compatibility (?) I made a separate attribute instead.

Unquestionably, customizable slug is the ultimate solution - but that went beyond my abilities (and requires a change in Ghost-Admin as well).

I'm definitely new to javascript (and handlebars) - this PR will likely to fail CI tests. Corrections are greatly appreciated!

@ErisDS
Copy link
Member

ErisDS commented Dec 11, 2018

Stepping in here because I've been looking at moving our "slugify" helper and wanted to make sure any open PRs around it were closed first.

The change to using our internal helper to generate the slug is absolutely spot on, thank you.

However, I don't understand the use case or reasoning for adding the base 64 encoded version. This would constitute a change to our theme API, which we only ever do with careful consideration. I understand that it's something to do with unidecode not producing a useful output, but if that's the case we should probably fix that, rather than adding new properties?

Part of the reason to move the helper out into a separate library is to make it easier to contribute to as I am aware it isn't performing well for non-roman languages right now. So, I'd suggest removing the base 64 change from this PR & getting this merged to fix #10258 asap.

I'll do my thing to move the helper and then it'll be easier to review the cases where it isn't outputting anything useful (which is a totally different problem to the one being solved here).

@TJYSunset
Copy link
Contributor Author

I understand that it's something to do with unidecode not producing a useful output, but if that's the case we should probably fix that, rather than adding new properties?

Being a native Chinese speaker and Japanese learner myself, I'm afraid current CJK romanization algorithms with acceptable quality are too computationally expensive (and will introduce a scarily large trained data). This article's introduction part should explain the challenges. Even machine learning based (?) Google Translate makes ridiculous tokenization frequently.

Naive base 64 may seems ugly, but at least it's cheap to calculate and always consistent + predictable.

@ErisDS
Copy link
Member

ErisDS commented Dec 12, 2018

Still, there are two separate problems being addressed in this PR. One problem is straightforward and the solution is accepted. The second problem needs to be addressed differently IMO - we need to get clarity on the problem and available solutions before making changes to the theme API.

I don't want to block one perfectly good solution - so it makes sense to split this out. Happy to do this our end though :)

@TJYSunset
Copy link
Contributor Author

Force pushed to remove last two commits.

@ErisDS ErisDS merged commit 2860dde into TryGhost:master Dec 13, 2018
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.

{{slug}} attribute in navigation.hbs skips most if not all non-ascii characters
2 participants