Skip to content

Feature/incorrect letters report - #4

Open
arrra wants to merge 13 commits into
masterfrom
feature/incorrect-letters-report
Open

arrra wants to merge 13 commits into
masterfrom
feature/incorrect-letters-report

Conversation

@arrra

@arrra arrra commented Dec 12, 2017

Copy link
Copy Markdown
Collaborator

User can see incorrect letters and ranked by color.

@arrra
arrra requested a review from mrap December 12, 2017 12:42
letter: key,
count: charsCount[key],
percent: (charsCount[key] / this.totalWrongCount * 100),
opacity: (charsCount[key] / this.totalWrongCount)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

dangling comma please

this.sortedIncorrectChars.push({
letter: key,
count: charsCount[key],
percent: (charsCount[key] / this.totalWrongCount * 100),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should save charsCount[key] / this.totalWrongCount to a variable since the value is used twice.

isRunning: function() {
return (this.state === LevelStates.Running);
},
incorrectCharsSorted: function() {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This method isn't named clearly. We use it to set and sort the this.sortedIncorrectChars.
A better name would be sortIncorrectChars since it's implying that the method does something vs returns something.

incorrectCharsSorted: function() {
let charsCount = this.result.missed_chars_count;

for(let key in charsCount){

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should assign charsCount[key] to a variable since it's used multiple times.

function startWpmTimer() {
wpmTimer = $interval(function() {
if (scope.lesson.state === LevelStates.Post) {
scope.lesson.incorrectCharsSorted();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think we should do this after stopping the timer (on the line below). It's a higher priority and is cheap efficiency-wise.

Comment thread client/src/assets/partials/lesson.html Outdated
</div>

<div class="lesson-incorrect-chars">
<div class="char" ng-repeat="char in lesson.sortedIncorrectChars track by $index"style="width:{{char.percent}}%; background:rgba(227, 69, 9, {{char.opacity}})">

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should avoid setting such a specific color here as it is smelly and hard to find since it is so separated from our styles.

Instead we should find a way to accomplish the same result via more maintainable code. (ask me)

@arrra
arrra force-pushed the feature/incorrect-letters-report branch from b9b56af to 139bd27 Compare December 13, 2017 14:21
letter: key,
count: incorrectCharCount,
percent: (incorrectPercent * 100),
opacity: incorrectPercent,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This shouldn't be in the model/data layer. It knows too much about what the UI does. Use char.percent in the UI to get the opacity


$progress-color: #2bc252;

$stats-bar-color: red;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should use the same red as wrong key. Maybe?

Comment thread client/src/assets/partials/lesson.html Outdated
</div>

<div class="lesson-incorrect-chars">
<div class="char" ng-repeat="char in lesson.sortIncorrectChars track by $index"style="width:{{char.percent}}%;">

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

lesson.incorrectChars not lesson.sortIncorrectChars

@arrra
arrra force-pushed the feature/incorrect-letters-report branch from 139bd27 to 453262f Compare December 13, 2017 15:26
@arrra
arrra force-pushed the feature/incorrect-letters-report branch from cbf4c78 to e89fcc0 Compare December 13, 2017 15:54

@mrap mrap left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Incorrect characters can render on multiple lines. Expected a single line.


$progress-color: #2bc252;

$incorrect-char-color: #F44336!important;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should be able to accomplish this without using !important. Avoid using !important at all costs, there is always a better way.

position: relative;
}

.char-background{

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Spacing

.pressable-key {
@extend .big-lettered;
@extend .blue-text;
color: #2196F3;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Do not hardcode. Use a variable.

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.

2 participants