Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report: celebrate your all 100's #5455

Merged
merged 18 commits into from
Jan 11, 2019
Merged

report: celebrate your all 100's #5455

merged 18 commits into from
Jan 11, 2019

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Jun 8, 2018

It's come up before. But it came up today: https://twitter.com/RobertJGabriel/status/1004604294102945793

The logic is, reward within the report if all the categories in your report have a 100. This can trigger even if you're only testing a single category. I considered enforcing that 2 or more categories are present, but that seems arbitrary.

(A further thing to explore: add additional fireworks per each included category. From a trickle up to a finale.)

example report:

@JoelEinbinder
Copy link
Contributor

.score100 .lh-lighthouse {
    animation: 4s linear infinite party-lighthouse;
}

@keyframes party-lighthouse {
    from{
        filter: hue-rotate(0deg);
    }
    to{
        filter: hue-rotate(360deg);
    }
}

@patrickhulce
Copy link
Collaborator

Ha, this is awesome!!! ❤️

@patrickhulce
Copy link
Collaborator

@hwikyounglee and I were just thinking it might be cool to go all out and replace the dark gray background and do a animation through the rainbow :D

.score100 .lh-header-bg {
  background: #f00;
  animation: 4s linear infinite party-background;
}

@keyframes party-background {
    from{
        filter: hue-rotate(0deg);
    }
    to{
        filter: hue-rotate(360deg);
    }
}

@hwikyounglee
Copy link

A suggestion to the excitement: shall we have a way to stop the animation to provide a control?
Options might be: play only once on load, stop when click the sky, stop on scrolling, or a combination of those.

@paulirish
Copy link
Member Author

@benschwarz
Copy link
Contributor

My vote is for lights coming out of he ligthouse as if there’s an epic party going on inside 👏

@hwikyounglee
Copy link

hwikyounglee commented Jun 8, 2018

  • For the collapsed header, either the text might need to be inverted or go back to the regular gray bar?
    image
  • I feel we should go with all category 100's for fireworks. For a solo cat 100, we could consider a confetti in each gauge in the future perhaps?
  • Also, wondering about the battery usage of the animation: if it consumes battery, I suggest the animation repeats x times (x=3~5?).
  • (added this comment later) It might feel a bit smoother if the fireworks start after the gauge svg animation completes.

Thanks Paul!

@ebidel
Copy link
Contributor

ebidel commented Jun 8, 2018

Lighthouse blue!? It's safer to sail in the day time.

screen shot 2018-06-08 at 1 06 40 pm

Also noticed a few scroll bugs.

Background area shows through header:

screen shot 2018-06-08 at 1 06 45 pm

Is the share icon meant to be see through?

screen shot 2018-06-08 at 1 07 13 pm

As @hwikyounglee pointed this out too. The text is hard to see:

screen shot 2018-06-08 at 1 06 49 pm

@paulirish
Copy link
Member Author

@paulirish paulirish requested review from egsweeny and removed request for vinamratasingal-zz November 6, 2018 02:48
@exterkamp
Copy link
Member

Took @JoelEinbinder's amazing party CSS and applied it to just the lighthouse light:
fe8fffb5-1a62-479e-b90f-f08752c56cb4
Yes? or very yes?

@patrickhulce
Copy link
Collaborator

oh VERY yes!

Copy link
Member Author

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

thx for bringing this back to life, @exterkamp

one nit, but this LGTM.

const scoreScale = this._dom.find('.lh-scorescale', scoresContainer);
scoreScale.style.opacity = `${1 - scrollPct}`;
scoreScale.style.transform = `translateY(${scrollPct * -scoreScalePositionDelta}px)`;
try {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we don't need this try/catch any longer.. pretty sure it was resolved somewhere else.. but IIRC it only applied in the single-category case.

Copy link
Member

Choose a reason for hiding this comment

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

I think we still need it. Even on master it throws errors when scrolling in single category reports. I can split this into another PR if it doesn't belong here, but I think this is still a bug.
master:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

lets just move a few lines inside of a

const scoreScale = this._dom.find('.lh-scorescale', scoresContainer);
if (scoreScale) { ... }

Copy link
Member

Choose a reason for hiding this comment

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

The error is on the find itself. This still throws, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

what i meant to say is.. "what a beautiful try/catch! let's keep it just like it is."

@exterkamp
Copy link
Member

Updated examples:

@connorjclark
Copy link
Collaborator

connorjclark commented Jan 10, 2019

Super fun I want this yesterday.

just gonna bring up @hwikyounglee's concerns about how this could be distracting.

A suggestion to the excitement: shall we have a way to stop the animation to provide a control?
Options might be: play only once on load, stop when click the sky, stop on scrolling, or a combination of those.

WDYT of stopping when clicking? (or better, some text somewhere saying Congrats on :100:! (click to disable animations) (disable forever)

This is under the assumption that a report of 100s still has useful information that developers might spent non trivial amounts of time reviewing the report.

@paulirish
Copy link
Member Author

Personally, I dont think it's worth adding user control to stop the animation. But Shane already did it so whatever. :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

yeah, agree about the interaction controls being unnecessary. Seeing the animation without cheating is going to be incredible rare in the first place, and you wouldn't have that report open for that long anyways because there's nothing much to look at in a perfect report but this new thing :)

@exterkamp
Copy link
Member

Copy link
Contributor

@egsweeny egsweeny left a comment

Choose a reason for hiding this comment

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

I dig it.

@paulirish paulirish merged commit 931eafe into master Jan 11, 2019
@paulirish paulirish deleted the fireworks branch January 11, 2019 18:58
@paulirish
Copy link
Member Author

🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants