Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

WIP Results progress bar #226

Draft
wants to merge 1 commit into
base: development
Choose a base branch
from
Draft

Conversation

nlkennedy
Copy link
Contributor

@nlkennedy nlkennedy commented Jun 26, 2019

Progress bar updates in real time with results corresponding to the results icon colors.

Screen Shot 2019-06-25 at 9 24 38 PM

Screen Shot 2019-06-25 at 9 24 59 PM

@nlkennedy nlkennedy requested a review from arscan June 26, 2019 01:26
count += 1
out << js_update_result(sequence, test_set, result, count, sequence.test_count, count, total_tests)
test_count += 1
out << js_update_result(sequence, test_set, result, count, sequence.test_count, test_count, total_tests, rerun)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of altering the js_update_result method to optionally print out previous test score, could you just loop through the previous tests and call js_update_result (the old version) on each here?

Copy link
Contributor Author

@nlkennedy nlkennedy Jun 26, 2019

Choose a reason for hiding this comment

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

Yes it could, but then the other information passed into js_update_result like sequence, _test_set, result, set_count, set_total, count would technically be incorrect (referring to the wrong test) and the console.log would get repeated for the "rerun" tests. As long as that's okay, do you still want me to update it?

Copy link
Contributor

@arscan arscan Jun 26, 2019

Choose a reason for hiding this comment

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

Yeah I guess my mental model was that it would just replay what already happened, so the user would be flashed very briefly everything that already run, and it would make it so you had one less special case in the code. But perhaps your method just makes more sense. I think you can just leave it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense too and it's an easy fix if you want to, I had just originally done it like that because js_update_result wouldn't know that some of the parameters were inaccurate but it wouldn't actually change anything for the user (only the console log).

@nlkennedy nlkennedy force-pushed the results_progress_bar branch from cf930f9 to 25b5181 Compare July 24, 2019 15:10
@nlkennedy nlkennedy force-pushed the results_progress_bar branch from 35a9f39 to 10a3c50 Compare August 9, 2019 13:51
@nlkennedy nlkennedy force-pushed the results_progress_bar branch from 10a3c50 to 20fd4c5 Compare August 9, 2019 14:19
@radamson radamson added the WIP Work in progress label Oct 10, 2019
@arscan arscan changed the title Results progress bar WIP Results progress bar Jan 11, 2021
@arscan arscan marked this pull request as draft January 11, 2021 14:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants