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

Tags transliteration #200

Closed
kifirkin opened this issue Nov 6, 2014 · 22 comments
Closed

Tags transliteration #200

kifirkin opened this issue Nov 6, 2014 · 22 comments
Milestone

Comments

@kifirkin
Copy link
Contributor

kifirkin commented Nov 6, 2014

What about using something like meteor-urlify2 or speakingURL for slug transliteration, because it just cant create tags in non English

@aaronjudd
Copy link
Contributor

That's a good idea. I like the looks of speakingURL. Could be a quick replacement of the existing slugify stuff I think.

@aaronjudd aaronjudd added this to the v0.2.1 milestone Nov 7, 2014
@kifirkin
Copy link
Contributor Author

I tried speakingURL and it works well, but I had to apply some modifications to it.
So the question is, wich way to use this lib better:

  • add it via npm
  • or just include js file in lib folder

I used file in lib folder but have to modify it to take context argument to be able use it inside server enviroment:

(function(root) {
...main code here...
        try {
            if (root.getSlug || root.createSlug) {
                throw 'speakingurl: globals exists /(getSlug|createSlug)/';
            } else {
                root.getSlug = getSlug;
                root.createSlug = createSlug;
            }
        } catch (e) {}
})(this);

was:

(function() {
...main code here...
        try {
            if (window.getSlug || window.createSlug) {
                throw 'speakingurl: globals exists /(getSlug|createSlug)/';
            } else {
                window.getSlug = getSlug;
                window.createSlug = createSlug;
            }
        } catch (e) {}
})();

@aaronjudd
Copy link
Contributor

We could make a package out of it, but I don't really think it's necessary. Putting it in lib, particularly since it's modified makes the most sense to me. If you want to make a PR for this, we can include this in the next release.

@pid
Copy link

pid commented Nov 11, 2014

speakingurl module is fixed, already released v0.17.0

@kifirkin
Copy link
Contributor Author

@pid thanks a lot :)

@aaronjudd
Copy link
Contributor

@pid oh great! @KEFIRCHIK, then I think we should release as a meteor package. I'll do that right now.

@aaronjudd
Copy link
Contributor

Created repo: https://github.com/ongoworks/meteor-speakingurl and published ongoworks:speakingurl

The package exports getSlug. To add: meteor add ongoworks:speakingurl

I'll add to the reaction-core dependancies, or @KEFIRCHIK you can do a PR.

@kifirkin
Copy link
Contributor Author

Thanks @aaronjudd! I have not so much time for now, and I'll make a PR as soon as possible!
btw, why u export getSlug only for server? I found some places on client where _.slugify() is used for autocompletion and I thought to place getSlug there also.

@aaronjudd
Copy link
Contributor

@KEFIRCHIK, yeah I was wondering about that at the time ;-) I'll update it.

@bstocks bstocks modified the milestones: v0.2.2, v0.2.1 Nov 18, 2014
@kifirkin
Copy link
Contributor Author

I commented at kefirchik/reaction-core@9b19f33, it fails on client if we load it via npm, cause there no Npm obj on clientside.
We need to manually add .js file on the client like this api.addFiles(".npm/package/node_modules/speakingurl.speakingurl.min.js", "client");
but how to do this if I add package via api.use("ongoworks:speakingurl@1.0.1", 'server'); ?

@kifirkin kifirkin reopened this Nov 21, 2014
@aaronjudd
Copy link
Contributor

You would just add to package.js api.use("ongoworks:speakingurl@1.0.2"); and then the files should be added already and getSlug exported.

I'm upgrading all the packages today, so I can test this later today (have a busy morning though).

I guess your point though is there isn't a client side version (because it's npm). so in that case I will change the package to have the min.js instead of using the npm install. that will take care of it.

@aaronjudd
Copy link
Contributor

I just published speaking url 1.0.2, and also added and upgraded package.js in reaction-core on v0.2.1 (and tested that getSlug is available on client)

@kifirkin
Copy link
Contributor Author

Oh thanks Ill test it now :)

@kifirkin
Copy link
Contributor Author

I ve just synced and pushed to my fork, but now app crashes with Error: inconsistent dependency constraint across unibuilds?, but I think its issue not related to reaction-core 😫

@aaronjudd
Copy link
Contributor

hmmm. That doesn't sound like a core issue.. sounds like some meteor package / build issue. I've downloaded and tested the package in RC and it's working for me.

@aaronjudd
Copy link
Contributor

@bstocks maybe you can test and see if you can replicate @KEFIRCHIK's error? maybe I have something else local

@kifirkin
Copy link
Contributor Author

Its weird, in my fork in package.js file speakingurl package added twice, I think I am doing smth wrong with merging from upstream :(

@aaronjudd
Copy link
Contributor

well I added it, and you added - it must not have seen them as the same line. probably safe to just revert to mine.

@kifirkin
Copy link
Contributor Author

I've commited it some time ago, before a couple of commits and I have no idea how to properly revert it.
maybe I just can commit the fix for it deleting mine changes?

@kifirkin
Copy link
Contributor Author

I know its bad practice to "flood" commits, when i have some more time I can do it better from the beginning: fork core package, make changes, commit to my frk and open pull request 😄

@aaronjudd
Copy link
Contributor

I think it's fine as long as the merge works (ie, you can flood your own fork all you want). I'll work through any conflicts when merging your pull requests. No worries.

@evliu
Copy link
Contributor

evliu commented Nov 22, 2014

@kerfichik, aaron can always squish commits in the merge :P haha, at my old company, the genius Chief Architect Officer did it all the time and formatted it really well

aaronjudd pushed a commit that referenced this issue Dec 3, 2015
First test for cart functionality.
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

No branches or pull requests

5 participants