Image Sequencing

Forums: 
Summary: 
sequence the images
tim's picture

Nice work! 

Thanks Tim,

Is there any suggestions for improvement   especially about the code structure, since I am a beginner here...

otacke's picture

Hi Jithin!

I am not an expert myself, but I have had a look at your code, and I have some general suggestions ...

  • You might want to have a look at the coding guidelines of H5P. For example, you should not use 4 spaces but 2 spaces for indenting. This is basically a matter of taste, but keeping it consistent will help others to understand H5P code across different source files.
  • Don't forget to add comments to your code. They should explain why you did something the way you did rather than translating your code to human language. This will not only help others to understand your code, but also yourself in 6 month or so ;-)
  • In semantics.json, you might want to use a "list" instead of a "group" for uploading images, see https://h5p.org/semantics#type-list. This wouldn't only make your code more flexible, but also shorter, and people won't have to upload all images again if they decide they could need one more image for the sequence.
  • You shouldn't use a JavaScript alert to show the score, I think. Why not have a look at some other content types that use the progress bar with the star for feedback?

I have some more proposals, but major things first ;-)

tomaj's picture

Hi Jithin,

Do you have an account on Github, where you can share your code (makes it easier to review it).

For writing good code, I recommend reading Clean code JavaScript.

In general I would recommend splitting up your attach method, to make it easier to read. Specially moving the call backs out to seperate class methods.

- Tom

fnoks's picture

Hi,

Thank you for sharing your content type. I have had a look at the code - here's my feedback:

  • You should not use an alert box. You could e.g. use the score bar, which is part of H5P.JoubelUI (github.com/h5p/h5p-joubel-ui).
  • It does not handle window resizing.
  • Usage of jquery UI: should probably use the jquery-ui H5P library instead. Or even better: don't use jquery UI at all :)
  • Semantics.json is unnecessary complex. Just use a list with min:3, max:6.
  • Use two spaces for indentation
  • Variable names are not consistent:
  1. Variables prefixed with $ should be jQuery objects
  2. Don't use underscore in variable names. E.g. instead of e_width use eWidth;
  3. Variables like "N" should not be used, since it doesn't say what it is.
  • Labels are hardcoded, i.e. this content type is not translatable
  • Some of the DOM elements have ids that will not be unique if several Image Sequencing H5Ps are placed on the same page. I do not see they are in use, so they probably could be removed? E.g.: dragZone, dropZone and titleZone

Let us know if anything is unclear - we'll help!

 

Great job on this content type, thanks!  I had a friend test this on his ipad and when he taps/drags the image, only the image drags, NOT the "card" with the text and the image.  If he taps and drags the "text" portion, the whole card works as expected.  Not sure if this can be fixed, but thought I'd share just in case!

Thanks again for sharing your work

tim's picture

Thanks for the feedback, Jimmy. The best way for this to be picked up by the Jithin, the creator of the content type, is to create an issue at the Github repository where the source code lives:
https://github.com/jithin-space/h5p-ImageSequencing

Note that you will have to create an account on Github if you don't have one already.

Thanks otacke,tomaj and fnoks for your feedbacks..

I am progressing on standardizing my code as per your feedbacks..I will upload the next version through github as soon as it is finished.. I am very excited to see the feedbacks and thanks for spenting time on my work  and for your helping mentality...

tomaj's picture

Happy to help! Good luck with your coding!

- Tom

Hi All,

could anyone please tell me how to use joubelUI in my content type...I am getting  TypeError: H5P.JoubelUI is undefined....

Here is what I did,

  • included following in the library json file...
  • {
          "machineName": "H5P.JoubelUI",
          "majorVersion": 1,
          "minorVersion": 2
        }
  • then a call within the main attach function...
  •    H5P.JoubelUI.createButton('button_name').appendTo($container);

please tell what I am missing?

fnoks's picture

It looks like you have done the correct thing, i.e added a dependency to JoubelUI, which means that library will be loaded when your H5P content type is loaded. Have you put the dependency in the right place? I.e, it should be inside preloadedDependencies:

"preloadedDependencies": [
  {
    "machineName": "H5P.JoubelUI",
    "majorVersion": 1,
    "minorVersion": 2
  }
]

If this is what your library.json looks like, you should verify version 1.2 of H5P.JoubelUI is installed on your system!

Let us know how it goes

screenshotHere is the link to my repository...it is not fully working...library json and scripts/image-sequencing.js are the files in consideration...

https://github.com/jithin-space/h5p-ImageSequencing

I beleive h5p.JoubelUI is installed in my drupal setup

 

tomaj's picture

Hi,

  1. Awesome that you got things moving forward :)
  2. The image in this blogpost is pointing locally to your computer. A trick that I do, is to use the Attachments field at the bottom of the comment form, to upload an image. And the copy the url of that image to be part of the post itself.

- Tom

I didn't get the idea at first...I just uploaded the attachment, use its url in the post and finally deleted the post..I think it is being displayed now

screenshot

It is working now...but I am not able to exactly what caused that..Anyway thanks for the help..

Hi all,

My Code for H5P.ImageSequencing Content type is being updated. I tried to my best to make use of all your suggestions..It is now properly indented, translatable, resizeable  and structured.. Please go through the code and  tell me if there exist any more modifications..

https://github.com/jithin-space/h5p-ImageSequencing

A screen shot of how a content created using that type is also being attached...

screenshot

sorry on my previous problem existed on a development branch and I shared the link for the master branch..

tomaj's picture

I tested Image Sequencing, and I it looks very cool! :)

But I get an exception from the timer in Chrome 58.

timer.js?opbf00:14 Uncaught TypeError: Cannot read property 'call' of undefined

    at new ImageSequencing.Timer (timer.js?opbf00:14)

    at ImageSequencing.self.attach (image-sequencing.js?opbf00:325)

    at Object.H5P.newRunnable (h5p.js?opbf00:821)

    at HTMLDivElement.<anonymous> (h5p.js?opbf00:132)

    at Function.each (jquery.js?opbf00:2)

    at init.each (jquery.js?opbf00:2)

    at Object.H5P.init (h5p.js?opbf00:92)

    at HTMLDocument.<anonymous> (h5p.js?opbf00:2012)

    at c (jquery.js?opbf00:2)

    at Object.fireWith [as resolveWith] (jquery.js?opbf00:2)

 

- Tom

Attachments: 
fnoks's picture

Tomaj: This happens because you are not including H5P.Timer. Nothing wrong with the content type :)

otacke's picture

Hi jithin!

Great that you're making progress! I think there are still some kinks to be ironed out, e.g. some global variables that should be avoided, because they can easily be overwritten. In my humble opinion, an awesome tool for getting some hints is JSHint. I embedded it in atom recently, and I love it for showing me possible issues instantly when typing my code.

fnoks's picture

Hi,

I see great improvements in your code. It is much tidier, and the way you have divided it into components makes it easy to follow. Great work!

I have done some testing, and especially when the images are of different height/width ratio, the UX-experience is a little strange, since the dropzones are jumping around while dragging. Generally, I think it is a little bit hard to know what I am supposed to do when seeing this content type the first time. If you'd like, I could get a UX designer to suggest a design for this content type - just let me know! I did find a bug when dropping one of the images outside the dropzones, then the image disappeared.

Below you'll find some comments about the code:

Your CSS is setting styles in a generic way (e.g. html, h1, h2, h3, h4, dd) and some of the selectors are too generic. This will be a problem since this content type is included using a DIV-tag, and those CSS-rules will then apply to other elements on the page. The best solution is probably to configure this H5P to be included using an iframe instead. This is done by adding the following to your library.json:

"embedTypes": [
  "iframe"
]

A drawback of displaying the H5P inside an iframe is that you need to inform the parent window about any resizing. This is done by triggering the resize event on the main class (ImageSequencing in your case). Eg: self.trigger('resize');

There are still some untranslatable strings in dropper.js ("Drop Here", "please drop")

Somewhere you are setting CSS rules using JS. This should not be necessary unless the values are dynamic/calculated. Instead, you should toggle classes. Here is an example:

self.$dropper.css('border', '2px solid red');

Could e.g instead be:

self.$dropper.addClass('image-sequence-incorrect');

and your CSS could actually set the style:

.image-sequence-incorrect {
  border: 2px solid red;
}

 

 

Hi All,

Now I am again with new code because the previously detected bug needed a detailed restructuring.

  • A new class called linkedList is introduced for easy handling in the shift.
  • A dropper is introduced into the list by default..for both correcting the bug and for initial user understanding..
  • Each Image is now attached with an image description that is displayed in their respective headers...( another hint for sequencing)
  • css gets modified  and each class name is now preceeded with 'sequencing' for better resolving

Please Once Again test my Code and make your suggestions...

I am  happy to get the suggestions also from a experienced UX designer as fnoks had mentioned...

https://github.com/jithin-space/h5p-ImageSequencing

Hello All,

I am not getting any responses for the past two weeks...please respond so that I can move forward onto more h5p tasks..sorry if you were too busy!!!!

fnoks's picture

Hi,

Our UX-designer has given some feedback, which is available here:
https://h5ptechnology.atlassian.net/browse/HFP-1156

Hi All,

Thanks for the feedback from the ux part. I would like to implement the changes as suggested. Could you please provide the design specs as mentioned in the response so that I can implement it quickly..

Hi,

Code Once again updated based on the suggestions from the ux designer. Major changes include , switch back to H5P jquery ui for smoother sortable functionality and removal of resize event...I think it works good automatically in all screen sizes...Please review the code ..

https://github.com/jithin-space/h5p-ImageSequencing

 

Hi all,

Does anyone know how to resize the images in this awesome content type? I tried to make changes in the h5p-image-sequencing.css to make the image smaller than 168 px, but it didn't do anything. Really appreicate any insights.