search

andyjac / top_10_albums

Displays top 10 albums for a highlighted musical artist in the browser
0 stars 0 forks source link

REVIEW: display tooltip #10

Closed bion closed 9 years ago

bion commented 10 years ago

:yellow_heart: = want :rotating_light: = need

bion commented 10 years ago

:yellow_heart: Might want to find a spinner that works better with different backgrounds, the white background works out poorly when you get the doge involved: spinner-with-doge

bion commented 10 years ago

:rotating_light: Found a concurrency-related bug! You've leveled up to another echelon of bug difficulty! Read on, brave engineer:

Mashing the mouse on the background causes the text to select and unselect really fast, with the unfortunate result of sending an ajax request each time the selection changes. If previous requests haven't gotten back yet, this can cause problems (see screen shot). I know this might seem like a fairly dickish thing for me to do and make a requirement of, but I'm doing my best to pretend I'm a QA engineer right now (Ben's job, here's a delightfully concise explanation of the basic concept behind good QA).

There's a couple ways to fix this, one would be to not make another request if one is currently being made. Doing this would be a form of mutual exclusion or 'mutex', or, more specifically, a lock. Those articles are helpful, but I think the easiest way to think of a lock is like a hall pass: only one at a time; you don't get to go until she's finished, etc.

Here's what the manifested bug looks like: overlapping-results

P.S. an easier solution that avoids the concurrency issue altogether is to just make sure there aren't any previous album lists already on the page when you append one:

  ...
  $('top albums uid class').remove();
  topAlbumsHTML.appendTo('html').fadeIn(200);
  ...
bion commented 9 years ago

:dizzy_face: merge conflict!

bion commented 9 years ago

https://help.github.com/articles/resolving-a-merge-conflict-from-the-command-line/

andyjac commented 9 years ago

hmm my git status seems to be okay...

On Tue, Nov 18, 2014 at 1:09 PM, Bion Johnson notifications@github.com wrote:

https://help.github.com/articles/resolving-a-merge-conflict-from-the-command-line/

— Reply to this email directly or view it on GitHub https://github.com/andyjac/lastfm_top_albums/pull/10#issuecomment-63545156 .

bion commented 9 years ago

your git status doesn't indicate merge conflicts

bion commented 9 years ago

Github indicates them with the "This pull request contains merge conflicts that must be resolved." message and gray icon below.

bion commented 9 years ago

Or, you'll find out about them when you attempt to merge or rebase locally.

andyjac commented 9 years ago

Sweet. I think I got it.

On Tue, Nov 18, 2014 at 1:19 PM, Bion Johnson notifications@github.com wrote:

Or, you'll find out about them when you attempt to merge or rebase locally.

— Reply to this email directly or view it on GitHub https://github.com/andyjac/lastfm_top_albums/pull/10#issuecomment-63546696 .

andyjac commented 9 years ago

You think this bad boy is ready to merge?

bion commented 9 years ago

You can definitely change it from "WIP" to "REVIEW" though

bion commented 9 years ago

Let me take a last glance at it before you hit that merge button.

On Tue, Nov 18, 2014 at 2:11 PM, Andrew notifications@github.com wrote:

You think this bad boy is ready to merge?

— Reply to this email directly or view it on GitHub https://github.com/andyjac/lastfm_top_albums/pull/10#issuecomment-63555106 .

andyjac commented 9 years ago

Word

bion commented 9 years ago

:sheep: :it:

andyjac commented 9 years ago

good to go?

bion commented 9 years ago

yup

On Tue, Nov 18, 2014 at 2:26 PM, Andrew notifications@github.com wrote:

good to go?

— Reply to this email directly or view it on GitHub https://github.com/andyjac/lastfm_top_albums/pull/10#issuecomment-63557318 .