Code review comment for lp:~lderan/ubiquity-slideshow-ubuntu/test

Revision history for this message
Dylan McCall (dylanmccall) wrote :

So, my feeling on this is each slideshow content version should be targeted towards a particular Ubuntu release. If it's necessary to grab the version ID automatically, my assumption is that means we're either shipping outdated content (because, really, a lot changes from one release to the next and we've already had a few instances where flavours ship with plainly incorrect slideshow content); or we're shipping very general content (which could be argued for…). Am I missing something? :)

Personally, I've always been an advocate of just not repeating the version number here at all since Ubiquity already communicates that and it isn't really relevant information at this stage, but I don't think I've won that argument.

Anyway, this is optional, so I don't mind, but for this change to work, we'll also need a change in a released or to-be-released version of Ubiquity that is similar to what you have in Slideshow.py. Has that happened at this stage? It might also be nice to have a sensible fallback in the event that the version ID isn't provided, too. Right now it looks like we'll get an empty space after a comma, and no version number. Perhaps wrap that whole thing in an element that disappears if there's no version number available, like "Xubuntu<span class="versionDetails">, <span class="releaseName"></span> <span class="versionId"></span></span>"?

Also - and I promise this is a tiny thing - your element classes are inconsistent. Please use dashes instead of camel case, there, like "release-name" and "version-id".

Thank you!

review: Needs Fixing

« Back to merge proposal