Theme previews by canstudios-louisem · Pull Request #1692 · adaptlearning/adapt_authoring

canstudios-louisem · 2017-08-02T13:26:51Z

This will search for a file called preview.jpg in the root of your theme and display it on the theme selection screen if there is one.

I will also make a PR to the vanilla theme with a pr for the preview.jpg one of our designers made.

taylortom · 2017-08-03T13:31:52Z

    float: left;
    position: relative;
-    background: url(/css/assets/theme_bg.png) top left no-repeat;
+    background: url(/images/theme_bg.png) top left no-repeat;


Is this correct? Getting a 403 error.

tomgreenfield · 2017-08-02T14:17:01Z


    postRender: function() {
+      var previewUrl = '/api/theme/preview/' + this.model.attributes.name + '/' + this.model.attributes.version;
+      var $previewLoc = this.$el.find('.theme-preview');


this.$el.find('.theme-preview') can just be written as this.$('.theme-preview')

tomgreenfield · 2017-08-03T13:47:59Z

+        statusCode: {
+          // if a preview image is found
+          200: function() {
+            $previewLoc.prepend('<img src="' + previewUrl + '"alt="Preview image"' + '/>');


Can the alt text be localised please?

Could also switch to the jQuery object syntax if you fancied it:

$previewLoc.prepend($('<img/>', { src: previewUrl, alt: '...' }));

taylortom · 2017-08-09T10:19:17Z

@canstudios-louisem when accessing the theme picker page with old themes (i.e. those without preview images), I get the following console errors:

11:17:58.042 jquery.js:9659 GET http://localhost:5000/api/theme/preview/adapt-theme-community/0.0.2 404 (Not Found)
11:17:58.047 jquery.js:9659 GET http://localhost:5000/api/theme/preview/adapt-contrib-vanilla/2.0.4 404 (Not Found)

Would it be possible to handle this scenario on the server-side, without returning an error?

canstudios-louisem · 2017-08-09T10:31:21Z

I will change the 404 response to a 204 would be fine and that won't give a console error and is a standard reponse for this kind of thing.

tomgreenfield · 2017-08-09T13:52:21Z

    },

    postRender: function() {
+      var previewUrl = '/api/theme/preview/' + this.model.attributes.name + '/' + this.model.attributes.version;


Very minor but could we use model.get() to access attributes here please, for consistency.

canstudios-louisem · 2017-08-10T10:48:41Z

@taylortom @tomgreenfield I have made those changes.

taylortom

Works for me

canstudios-louisem · 2017-08-10T14:57:26Z

Before release we also need to update https://github.com/adaptlearning/adapt_framework/wiki/Developers-Guide:-Theme

tomgreenfield

Looking good – theme previews are now pulling in fine and the absent ones aren't 404'ing.

I've noticed that the sample image doesn't fit entirely with the current styling. Can we take this opportunity to standardise the image and styling to fit a 16:9 ratio? This would give an optimal resolution of 275 x 155.

It would also be great to fall back to a default icon, like we do in the asset manager.

canstudios-louisem · 2017-08-18T08:42:12Z

@tomgreenfield I've just tested 16:9 and it's quite short which means you can't really see any components and the header I think we should stay at 275x205. I will add the same default icon as in assets though.

canstudios-louisem · 2017-08-18T14:31:05Z

@tomgreenfield and @taylortom do you want to re approve as I've pushed a few things since you did?

taylortom · 2017-08-21T10:42:26Z

+            $previewLoc.prepend($('<img/>', { src: previewUrl, alt: Origin.l10n.t('app.themepreviewalt') }));
+          },
+          204: function() {
+            $previewLoc.prepend($('<i/>', { class: 'fa fa-file-image-o' }));


Very minor suggestion: I wonder if we can use something more theme-related for the icon here, as we use the image file one in various other places in the app's asset management. Best font awesome icon I can find is the paintbrush (there may be something better):

image icon looks more like it is related to assets

That's fine by me ive asked one of our creatives to see if they know anything better than the paintbrush as im not sure its quite right.

Cool, thanks Louise!

tomgreenfield · 2017-08-21T15:43:27Z


 .theme-list-item {
-  height: 175px;
+  height: 280px;


The sample image still doesn't display fully with the styling provided. We'll need an extra 15px added to the height to fit with the padding suggested in the comment below.

tomgreenfield · 2017-08-21T15:49:42Z

+          color:rgb(214, 242, 249);
+          font-size: 50px;
+          padding-top: 70px;
+          padding-left: 116px;


Instead of using padding, please can we centre the icon dynamically using a CSS transform?

position: absolute; top: 50%; left: 50%; transform: translate(-50%, -50%);

With a set height on the container (205px if we're matching the sample image).

tomgreenfield · 2017-08-21T15:59:21Z

  <div class="theme-header">
    <div class="display-name"><h4 style="color: {{foreground_color.default}}">{{displayName}}</h4></div>
    <div class="description">{{description}}</div>
+    <div class="theme-preview"></div>


Please can the preview <div> be moved to sit after the header <div>?

This will allow us to have consistent padding – none on the inner, 20px on the header and none on its children.

tomgreenfield · 2017-08-21T16:05:36Z

+            $previewLoc.prepend($('<img/>', { src: previewUrl, alt: Origin.l10n.t('app.themepreviewalt') }));
+          },
+          204: function() {
+            $previewLoc.prepend($('<i/>', { class: 'fa fa-file-image-o' }));


We need quotes around class since it's a reserved word in JavaScript.

canstudios-louisem · 2017-08-22T15:12:03Z

@taylortom @tomgreenfield I've pushed an update with those changes. I added the paintbrush but our designer also made this thoughts? I'm not set on either the paintbrush is one less image file tho.

taylortom · 2017-08-22T15:41:47Z

Great thanks @canstudios-louisem, looks good.

I have to admit that the linked image icon doesn't mean anything to me, I'd never guess it was related to appearance/theming. Happy to go with the general consensus though.

tomgreenfield

Sorry, minor comment again!

tomgreenfield · 2017-08-25T13:44:16Z


+  &-inner {
+    .theme-header {
+      padding: 20px 0;


Can the padding be made equal so we avoid text hitting the side of the container? We then won't need it declared for .display-name or .description.

tomgreenfield · 2017-08-25T13:46:03Z

+            $previewLoc.prepend($('<img/>', { src: previewUrl, alt: Origin.l10n.t('app.themepreviewalt') }));
+          },
+          204: function() {
+            $previewLoc.prepend($('<i/>', { 'class': 'fa fa-paint-brush' }));


Happy with the paintbrush icon, @taylortom.

canstudios-louisem · 2017-08-25T14:51:12Z

sorted @tomgreenfield

canstudios-louisem added this to the 0.4.0 Refactor milestone Aug 2, 2017

taylortom reviewed Aug 3, 2017

View reviewed changes

tomgreenfield reviewed Aug 3, 2017

View reviewed changes

canstudios-louisem mentioned this pull request Aug 4, 2017

preview image for AT theme preview adaptlearning/adapt-contrib-vanilla#213

Closed

tomgreenfield reviewed Aug 9, 2017

View reviewed changes

canstudios-louisem requested a review from canstudios-nicolaw August 10, 2017 10:48

taylortom approved these changes Aug 10, 2017

View reviewed changes

canstudios-louisem force-pushed the theme-previews branch from 1cbc40b to 1163f7b Compare August 15, 2017 09:07

tomgreenfield approved these changes Aug 16, 2017

View reviewed changes

canstudios-louisem force-pushed the theme-previews branch from 1163f7b to 3048f3a Compare August 18, 2017 09:08

canstudios-louisem added 5 commits August 18, 2017 14:49

Theme preview extracted from can code and refactored

a43b56a

styling brought in line with menu editor

01d9ca0

jquery better utilised and at text localised

182add1

getter used on models and 204 is now response on no preview not 404

7e63aad

default image

07ba658

canstudios-louisem force-pushed the theme-previews branch from 3048f3a to 07ba658 Compare August 18, 2017 13:49

canstudios-nicolaw approved these changes Aug 18, 2017

View reviewed changes

taylortom reviewed Aug 21, 2017

View reviewed changes

tomgreenfield reviewed Aug 21, 2017

View reviewed changes

tomgreenfield reviewed Aug 25, 2017

View reviewed changes

updated from feedback changed icon to paintbrush

1433395

padding feedback

a19acf9

canstudios-louisem force-pushed the theme-previews branch from 2886e78 to a19acf9 Compare August 25, 2017 14:50

tomgreenfield approved these changes Aug 25, 2017

View reviewed changes

taylortom approved these changes Aug 26, 2017

View reviewed changes

canstudios-nicolaw approved these changes Aug 29, 2017

View reviewed changes

canstudios-louisem merged commit 7fbab8d into adaptlearning:develop Aug 29, 2017

canstudios-louisem deleted the theme-previews branch August 29, 2017 08:08

Conversation

canstudios-louisem commented Aug 2, 2017

Uh oh!

taylortom Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylortom commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

canstudios-louisem commented Aug 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canstudios-louisem commented Aug 10, 2017

Uh oh!

taylortom left a comment

Choose a reason for hiding this comment

Uh oh!

canstudios-louisem commented Aug 10, 2017

Uh oh!

tomgreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

canstudios-louisem commented Aug 18, 2017

Uh oh!

canstudios-louisem commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomgreenfield Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canstudios-louisem commented Aug 22, 2017

Uh oh!

taylortom commented Aug 22, 2017

Uh oh!

tomgreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canstudios-louisem commented Aug 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

taylortom Aug 3, 2017 •

edited

Loading

taylortom commented Aug 9, 2017 •

edited

Loading

tomgreenfield Aug 21, 2017 •

edited

Loading