Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Repeater Thumbnail Alignment #873

Merged
merged 1 commit into from
Dec 3, 2014
Merged

Conversation

kevinparkerson
Copy link
Contributor

fixes #851 by adding a thumbnail_alignment option

@kevinparkerson kevinparkerson added this to the 3.4.0 milestone Nov 24, 2014
@kevinparkerson
Copy link
Contributor Author

Note: we should look this over before merging it and decide if we really want this approach. While the justified thumbnails look great with full rows... with a row of say, 3 thumbnails, it looks a bit odd.


var eachFunc = function(){
//these dumb functions are necessary because lint yells when a function is in a loop :\
Copy link
Contributor

Choose a reason for hiding this comment

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

i would encourage you to research this to see if it is "dumb" or not. we can tell jshint to ignore if we determine it is dumb but i think we should keep it. first find after googling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. The comment is meant to indicate why this potentially unusual decision was made. That being said, it is not necessarily "dumb" so I'll remove that word :)

@kevinparkerson
Copy link
Contributor Author

@swilliamset - updated. Still seeing this though: https://dl.dropboxusercontent.com/u/12146987/justified.png Any opinion on that? @interactivellama, your opinion here would be helpful as well

@swilliamset
Copy link
Contributor

Technically still justified. Maybe centering or providing an option to left/center/justify/right. Each has it's pros and cons but work as expected.

@kevinparkerson
Copy link
Contributor Author

@swilliamset - interesting... could be a thumbnail_alignment option with settings 'left / center / justify / right / none' instead of just justify...

@swilliamset
Copy link
Contributor

bingo i like that. what should the default be?

@kevinparkerson
Copy link
Contributor Author

I made the default 'justify' but if others think another setting should be default I can easily change it :) Adding unit tests here in a sec

@kevinparkerson kevinparkerson changed the title Repeater Thumbnail Justify Repeater Thumbnail Alignment Dec 2, 2014
@@ -19,6 +19,37 @@
padding: 6px;
width: 100%;

&.align-center, &.align-justify, &.align-left, &.align-right {
position: relative;
text-align: justify;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not specify this and then override it below. Let's just add a &.align-justify rule below.

… justify solution

repeater-thumbnail-justify: fix to 'thumbnail_setSelectedItems' after making justify changes, which also makes unit tests pass

repeater-thumbnail-justify: yet another minor fix because lint yells about something dumb

repeater-thumbnail-justified: minor touchups after review

repeater-thumbnail-justified: additional function name improvements

repeater-thumbnail-justified: converted to thumbnail_alignment option instead so now center / justify / left / right (as well as 'none' or false) is supported

repeater-thumbnail-justified: added unit tests. also added "spacer" class to injected spans

repeater-thumbnail-justified: added default font-size for thumbnail
@kevinparkerson kevinparkerson force-pushed the repeater-thumbnail-justify branch from 083548c to c5e7e74 Compare December 3, 2014 17:10
swilliamset pushed a commit that referenced this pull request Dec 3, 2014
@swilliamset swilliamset merged commit a9ad5ae into master Dec 3, 2014
@swilliamset swilliamset deleted the repeater-thumbnail-justify branch December 3, 2014 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants