-
Notifications
You must be signed in to change notification settings - Fork 229
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
New snippets for Spanish lesson index #625
Conversation
@vgayolrs can you please push commits here: https://github.com/programminghistorian/jekyll/blob/lesson-filter-es-fix/_data/snippets.yml#L135-L140 Please add your commits to the |
Thanks @mdlincoln . I'm on it. Let me review carefully some details. I'll let you know when I'm done. |
ok @mdlincoln , I think it's ready to merge. |
Thank you @vgayolrs. We still need to fix the Javascript issue before these snippets can be displayed, unfortunately. I'll keep working on it. |
You're welcome @mdlincoln . Let me know if there's anything else I can do. Unfortunately, no javascripts at the moment. |
uses hidden input element that jquery can then call
So @walshbr I know no javascript whatsoever but I think I hacked my way through this. As described in the original issue, the problem is that we can't use liquid includes within lessonfilter.js, so we need to find another way to pass the proper text labels through to javascript. I think I've done that here. One question: do you know how to properly set |
That what you had in mind @mdlincoln? Check out the last commit where I set the variables at the bottom of the js file. One issue with how I did this, I think, is that it sets the labels on each page load. So if the file gets loaded on a page where those labels don't exist we'd get some unexpected behavior. But it appears that this js file is only getting included on the lessons page anyway, so that problem never arises. And great job figuring it out! I couldn't quite wrap my head around what you meant in the original note on this issue, so I'm glad that you figured it out. |
Ah I see - I'd tried saving them to variables somewhere else in the JS and it wasn't working. I think this is ready for merging if you agree @walshbr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! We can open separate tickets if anything arises. But seems like the functionality is working as expected, and I'm not seeing errors in the console anywhere.
While waiting to figure out the JS problems on #623, we can at least translate these snippets.Closes #623