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

Different image on cardback v1 #11

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

supriyarajgopal-spi
Copy link

No description provided.

supriyarajgopal-spi and others added 8 commits October 24, 2016 15:45
- Code indentation
- Camelcase for Javascript variables as per coding convention
- Removed unnecessay boolean variable to check if same image should be used on both front & back
- Accessing object attributes as per coding convention
Copy link
Member

@thomasmars thomasmars left a comment

Choose a reason for hiding this comment

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

Looks good so far :) There are some parts that needs to be cleaned up and the animation needs some love before it can be merged in. Please have a look at the comments, and down hesitate to ask if you need any help with the implementations.

You also need to add the object to the language files, when making changes to semantics.json. This means adding any semantic fields that was added to semantic.json to all languages files in the language/ folder (i.e. nb.json, fr.json, etc.). It should be fairly obvious how to add this by studying the pattern for other translations and follow that style.

self.truncateRetryButton();
self.resizeOverflowingText();
}
self.addTipToCard($c, turned ? 'front' : 'back');
Copy link
Member

Choose a reason for hiding this comment

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

Please fix indentation here, use 2 spaces instead of tab.

@@ -21,8 +21,8 @@
"type": "list",
"widgets": [
{
"name": "VerticalTabs",
"label": "Default"
"name": "VerticalTabs",
Copy link
Member

Choose a reason for hiding this comment

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

Fix indentation

}
]
}
{
Copy link
Member

Choose a reason for hiding this comment

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

indentation

self.turnCard($(this).parents('.h5p-dialogcards-cardwrap'));
}).appendTo($cardFooter);
//Create Turn button only if either answer or back image exists
if(card.answer != "" || card.backImage != undefined)
Copy link
Member

Choose a reason for hiding this comment

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

card is undefined for self.createCardFooter() in line L#315, which makes this crash

@@ -490,9 +492,14 @@ H5P.Dialogcards = (function ($, Audio, JoubelUI) {
// Check if card has been turned before
var turned = $c.hasClass('h5p-dialogcards-turned');

//Show the front image only if a back image is not uploaded.
//P.S. Adding this snippet after changeText() caused image sizing problems
if (self.params.dialogs[$card.index()].backImage.length) {
Copy link
Member

Choose a reason for hiding this comment

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

If backImage is not defined we get a JS error here

Copy link
Member

Choose a reason for hiding this comment

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

Also backImage is not an array, it is an object, so we should probably not check its length, I suppose checking that it is defined would suffice.

*/
C.prototype.changeImage = function ($card, imgSrc) {
var $cardImage = $card.find('.h5p-dialogcards-image');
if(imgSrc != undefined)
Copy link
Member

Choose a reason for hiding this comment

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

Please start curly brace on the same line as the if-statement

* @param $card
* @param imgSrc
*/
C.prototype.changeImage = function ($card, imgSrc) {
Copy link
Member

Choose a reason for hiding this comment

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

Image is changed momentarily once we click the "turn" button, image should change when the card has transitioned halfway through.

"description": "Optional image for the card. (The card may use just an image, just a text or both)"
},
{
"name": "backImage",
Copy link
Member

Choose a reason for hiding this comment

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

backImage needs translations to all languages (you don't actually have to translate them, just make sure the languages structures are correct)

@mandrasch
Copy link

Including this would be nice :-) (I'm not capable right now regarding my h5p beginner skills)

Also mentioned in forum: https://h5p.org/node/70099

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.

3 participants