cs50 / compare50

This is compare50, a fast and extensible plagiarism-detection tool.
GNU General Public License v3.0
195 stars 49 forks source link

Issues 55 and 59 #67

Closed jsarchibald closed 4 years ago

jsarchibald commented 4 years ago

Resolves #55 by adding a CSS rule to .file_name.

Resolves #59 by changing go_to_next_group to a more abstracted function called go_to_adjacent_group. This function takes in a direction, "n" (next) or "p" (previous) as an optional argument (used for key presses), and will attempt to discern the direction from the event target's ID if not provided (as in the case where the event is called from a click event handler).

Adds a variable on the same scope as group_index called last_move, to keep track of whether the last move was to the next or previous group (this is due to the existing design of how group_index is incremented; I needed a way to account for whether the group_index we're moving to is 0 because we were on the last element using the Next button, or because we're moving to the previous group and should actually go there).

Adds a variable scoped under go_to_adjacent_group called index_increment which determines whether we should increment group_index positively (in the case of next button) or negatively (previous button). I reasoned that this was cleaner than incorporating a ternary conditional into the existing one-liner for incrementing group_index.

Adds a button to match_page.html for the next group button. This is within a new btn-group; previous #next_group styling was changed to .matches_group so as to provide the same for both buttons.

Adds an event listener on the left arrow key to move to the previous group. Did not change the existing space bar event listener to move to the next one, but for consistency also added a right arrow key event listener to move to the next group.

I wasn't sure about the style standards for JS in this repo, but using braces even for one-line conditional predicates seemed to be the norm, so I did that. Looking forward to any feedback you may have.

Jelleas commented 4 years ago

Works great, but I think we can make this a little simpler:

There's only two erroneous situations here, either we go to "previous" and underflow (group_index < 0) or "next" and overflow (group_index >= sorted_groups.length). In both cases we know what group_index should become. So it's probably simpler to drop the last_move and modulo here, and replace it with if + else-if instead.

I'm not a huge fan of using strings to represent states (but there's a time and place). Given that this is just a private function, probably okay to directly use -1 and 1 for direction instead of "p", "n".

Using an anonymous (arrow) function below makes it so go_to_adjacent_group doesn't need to know about buttons: next_group_button.addEventListener("click", go_to_adjacent_group); => next_group_button.addEventListener("click", (event) => go_to_adjacent_group(event, 1));

You can use the arrow keys to scroll left-right in the browser which makes them not great for overloading. Probably best to switch to something else, like [ ] perhaps?

jsarchibald commented 4 years ago

Will make these changes ASAP!

jsarchibald commented 4 years ago

Made all the changes requested, plus fixed the issue of not highlighting the first group at initialization by calling go_to_adjacent_group after initializing group_index.

jsarchibald commented 4 years ago

Also, this resolves #64.