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

Refactor taxonomy js tree as mentioned in #2129 #2192

Closed
wants to merge 3 commits into from
Closed

Refactor taxonomy js tree as mentioned in #2129 #2192

wants to merge 3 commits into from

Conversation

codeodor
Copy link
Contributor

@codeodor codeodor commented Nov 7, 2012

No description provided.

@codeodor
Copy link
Contributor Author

codeodor commented Nov 7, 2012

Just a note: I chose to do this on the 1-2-stable branch instead of master, because I am more familiar with it, plus I was having trouble getting tests and a dummy site running on master when I tried there first (before making any changes).

@derfarg
Copy link

derfarg commented Nov 27, 2012

@codeodor im using 1-2-stable in our productive client shops and in all of this. The js tree in admin dont work. This job is done?

@codeodor
Copy link
Contributor Author

Wow, I didn't get this notification via email ... just saw it now, so sorry for the big delay in response!

Yes, I believe it should be done. The tests were passing and it's working on my installation...

What issue were you experiencing? Maybe a description will help me figure out what's wrong?

PS: Just to be clear, the issue you were having is with 1-2-stable as-is, or with the code I am requesting be merged here? If the issue you're having is with 1-2-stable as it exists before these commits, I doubt these commits will help. (But I'd be willing to look at the issue anyway and see if I can lend a hand in fixing it)

@iloveitaly
Copy link
Member

It seems like a apostrophe in the name will cause the tree to break.

@codeodor
Copy link
Contributor Author

codeodor commented Feb 2, 2013

Thanks for the insight. I'm heading out of town for a conference where I don't think I'll get any free time to look, but I'll create a fix for the problem when I get back at the end of next week.

@radar
Copy link
Contributor

radar commented Feb 3, 2013

@iloveitaly: Still? Even in 1-3-stable and master?

@radar
Copy link
Contributor

radar commented Feb 3, 2013

I am hesitant to merge this pull request because I've already done some refactoring for it within the split_core branch; notably, moving the responsibility of generating the JSON hash to the API, rather than having it in the view or in the model.

Is there anything else this PR addresses?

@iloveitaly
Copy link
Member

@radar Not sure about 1-3-stable or master... I'm working off of 1-2-stable right now. Could this fix be backported onto 1-2-stable?

@codeodor
Copy link
Contributor Author

codeodor commented Feb 4, 2013

@radar - The only other important thing this PR does aside from move the JSON generation is refactor the JS from straight js in a script tag to be called from a function, thereby making the code usable in other places.

When I get back to work, I could look through the split_core branch and do it there instead if you'd rather just close this one.

@radar radar closed this Feb 6, 2013
@radar
Copy link
Contributor

radar commented Feb 6, 2013

Yes. Please look through the changes on split_core and see if you can
improve the code there.

On Tue, Feb 5, 2013 at 6:54 AM, Sammy Larbi notifications@github.comwrote:

@radar https://github.com/radar - The only other important thing this
PR does aside from move the JSON generation is refactor the JS from
straight js in a script tag to be called from a function, thereby making
the code usable in other places.

When I get back to work, I could look through the split_core branch and do
it there instead if you'd rather just close this one.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2192#issuecomment-13095720.

This was referenced Feb 26, 2013
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.

None yet

4 participants