Skip to content

animate developer quotes. #342

Merged
merged 1 commit into from Dec 6, 2012

4 participants

@stephenplusplus
TasteJS member

I thought the homepage could use a little fanciness. I just made the developer quotes transition between each other more gently, as opposed to the removal/recreating/reinserting of the quotes every 25 seconds. Tried to tighten up the spending on the DOM and elaborate the functionality a bit. First PR, please be patient with me! If I need to do something differently, please let me know.

@sindresorhus
TasteJS member

Congrats on your first PR :tada:

Can you make it random again and follow the existing code style?

Otherwise this looks good to me. Thanks :)

@stephenplusplus
TasteJS member

Thank you!

I forgot to explain why I removed the random functionality in the first place. With only 4 quotes, it seemed to make more sense just to remove it altogether. However, with my last commit, I've added it back in-- kind of. Now, instead of always displaying a random quote, it will only start with a random quote.

So instead of:

Rebecca, Paul, Paul, Rebecca, Justin, Paul, Michael, Rebecca, Paul

it would be:

Michael, Justin, Rebecca, Paul, Michael, Justin, Rebecca, Paul
or
Justin, Rebecca, Paul, Michael, Justin, Rebecca, Paul, Michael
(etc)

If the original functionality is important, I can add that back in, too.

As far as code style, it was hard to detect a pattern in this JS. However, I went through my code and elaborated the variable names a bit, and did some overall less weird stuff, that maybe was hard to look at from my first commit.

If I can do anything else, I'll be happy to.

Thanks for your help!

@sindresorhus sindresorhus commented on an outdated diff Dec 6, 2012
site/js/main.js
- function update() {
- var el = data[ Math.floor( Math.random() * data.length ) ];
+ text.innerText = quote.quote;
+ link.innerText = quote.person.name;
+ link.setAttribute('href', quote.link);
+ img.setAttribute('src', quote.person.gravatar);
@sindresorhus
TasteJS member
sindresorhus added a note Dec 6, 2012

Just use the jQuery methods, the speed benefit is moot anyway. Also don't see the point of those temp vars link , img etc, just use them directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff Dec 6, 2012
site/js/main.js
+ if (!quoteElems[activeQuoteIndex]) {
+ quoteElems[activeQuoteIndex] = $(quotes[activeQuoteIndex]);
+ }
+
+ var activeQuoteElem = quoteElems[activeQuoteIndex];
+
+ fader(prevQuoteElem, activeQuoteElem);
+
+ ++activeQuoteIndex < quoteCount? activeQuoteIndex : activeQuoteIndex = 0;
+ prevQuoteElem = activeQuoteElem;
+ };
+ return swap();
+ };
+
+ $.fn.quote = function(quoteObjects) {
+ var container = this.parent(),
@sindresorhus
TasteJS member
sindresorhus added a note Dec 6, 2012

I would prefer it if the element the plugin selected was the container. So this instead of this.parent().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff Dec 6, 2012
site/js/main.js
+ quoteElems[activeQuoteIndex] = $(quotes[activeQuoteIndex]);
+ }
+
+ var activeQuoteElem = quoteElems[activeQuoteIndex];
+
+ fader(prevQuoteElem, activeQuoteElem);
+
+ ++activeQuoteIndex < quoteCount? activeQuoteIndex : activeQuoteIndex = 0;
+ prevQuoteElem = activeQuoteElem;
+ };
+ return swap();
+ };
+
+ $.fn.quote = function(quoteObjects) {
+ var container = this.parent(),
+ template = this[0].outerHTML,
@sindresorhus
TasteJS member
sindresorhus added a note Dec 6, 2012

Again, just use jQuery methods. Speed implications here are moot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus
TasteJS member

If the original functionality is important, I can add that back in, too.

It's not important, but I would prefer to have it fully random. I like it that there's a random quote on each load, and that the following quotes are in random order too. People are likely to stick around for one, or maybe two, and it would be nice if neither of those are the same each time.

@stephenplusplus
TasteJS member

Thank you for taking the time to review the code and the feedback. Hopefully this latest commit has satisfied your requests.

Regarding the random quotes, it should now return a random quote that wasn't one of the last quotesLength quotes to be displayed. So, it isn't entirely random, but almost a hybrid of both of our ideas, which I think is the best option.

Thanks again for all of the feedback, I greatly appreciate it!

@sindresorhus
TasteJS member

Are you sure you pushed the right one? Looks like the first commit.

@stephenplusplus
TasteJS member

Yep, that was a mistake. Just pushed the new update. Thanks again for your patience!

@sindresorhus sindresorhus commented on an outdated diff Dec 6, 2012
site/js/main.js
- update();
- };
+ if (quoteCount > ++quoteElemCount) createQuoteElems();
@sindresorhus
TasteJS member
sindresorhus added a note Dec 6, 2012

braces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff Dec 6, 2012
site/js/main.js
+ var fader = function (fadeOut, fadeIn) {
+ var fadeOutCallback = function(){
+ fadeIn.fadeIn(500, fadeInCallback);
+ };
+
+ var fadeInCallback = function(){
+ window.setTimeout(swap, animSpeed);
+ };
+
+ fadeOut.fadeOut(500, fadeOutCallback);
+ };
+
+ var quotes = container.children(),
+ selectRandomQuoteIndex = Quotes.random(quotes),
+ quoteElems = {},
+ quoteCount = quotes.length,
@sindresorhus
TasteJS member
sindresorhus added a note Dec 6, 2012

not used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus
TasteJS member

Also use tab indentation.

Otherwise looks splendid to me. Great work! :)

@stephenplusplus
TasteJS member

Thank you! Think I got it this time :)

@sindresorhus sindresorhus merged commit faa859e into tastejs:gh-pages Dec 6, 2012
@sindresorhus
TasteJS member

Boom :bomb:

@sindresorhus
TasteJS member

Thanks for this :star2:

@passy
TasteJS member
passy commented Apr 30, 2014

:heart_decoration:

@sindresorhus sindresorhus referenced this pull request in stephenplusplus/ama Jul 10, 2015
Closed

Who were/are your mentors? #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.