-
Notifications
You must be signed in to change notification settings - Fork 167
Max value for participant slider on results page #2542
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,9 @@ | |
| RESULTS_WARNING_COUNT = 4 | ||
| RESULTS_WARNING_PERCENTAGE = 0.5 | ||
|
|
||
| # when this value is selected, the filter acts as an open end (no upper limit) | ||
| PARTICIPANT_FILTER_MAX_VALUE = 150 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have to put at least something like "results" in the name, otherwise this setting sounds more important than it is; the comment could also use some more context |
||
|
|
||
| ## percentages for calculating an evaluation's total average grade | ||
| # grade questions are weighted this much for each contributor's average grade | ||
| CONTRIBUTOR_GRADE_QUESTIONS_WEIGHT = 4 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,16 +147,20 @@ abstract class DataGrid { | |
| row.filterValues.get(name)?.some(rowValue => rowValue === filterValue), | ||
| ), | ||
| ); | ||
| const isDisplayedByRangeFilters = [...this.state.rangeFilter].every(([name, bound]) => | ||
| row.filterValues | ||
| .get(name) | ||
| ?.map(rawValue => parseFloat(rawValue)) | ||
| .some(rowValue => rowValue >= bound.low && rowValue <= bound.high), | ||
| ); | ||
| const isDisplayedByRangeFilters = this.checkRangeFilters(row); | ||
| row.isDisplayed = isDisplayedBySearch && isDisplayedByFilters && isDisplayedByRangeFilters; | ||
| } | ||
| } | ||
|
|
||
| protected checkRangeFilters(row: Row): boolean { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check for what? |
||
| return [...this.state.rangeFilter].every(([name, bound]) => | ||
| row.filterValues | ||
| .get(name) | ||
| ?.map(rawValue => parseFloat(rawValue)) | ||
| .some(rowValue => rowValue >= bound.low && rowValue <= bound.high), | ||
| ); | ||
| } | ||
|
|
||
| protected sort(order: [string, "asc" | "desc"][]) { | ||
| this.state.order = order; | ||
| this.sortRows(); | ||
|
|
@@ -426,6 +430,22 @@ export class ResultGrid extends DataGrid { | |
| this.resetOrder = resetOrder; | ||
| } | ||
|
|
||
| protected checkRangeFilters(row: Row): boolean { | ||
| return [...this.state.rangeFilter].every(([name, bound]) => { | ||
| const sliderEntry = this.filterSliders.get(name); | ||
| const isOpenEnd = sliderEntry?.slider.isOpenEnd() ?? false; | ||
|
|
||
| return row.filterValues | ||
| .get(name) | ||
| ?.map(rawValue => parseFloat(rawValue)) | ||
| .some(rowValue => { | ||
| const isWithinLow = rowValue >= bound.low; | ||
| const isWithinHigh = isOpenEnd || rowValue <= bound.high; | ||
| return isWithinLow && isWithinHigh; | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| public bindEvents() { | ||
| super.bindEvents(); | ||
| for (const [name, { checkboxes }] of this.filterCheckboxes.entries()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,16 +17,18 @@ export class RangeSlider { | |
| private readonly rangeLabel: HTMLSpanElement; | ||
| private allowed: Range = { low: 0, high: 0 }; | ||
| private _value: Range = { low: 0, high: 0 }; | ||
| private configurableMaxValue?: number; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to put "configurable" in there. It is not configurable after we passed the value initially. And the possibility of its absence is already clear from the type. Further, why is this a number at all? Shouldn't this number always equal |
||
|
|
||
| private debounceTimeout?: number; | ||
|
|
||
| public constructor(sliderId: string) { | ||
| public constructor(sliderId: string, maxValue?: number) { | ||
| this.rangeSlider = selectOrError<HTMLDivElement>("#" + sliderId); | ||
| this.lowSlider = selectOrError<HTMLInputElement>("[name=low]", this.rangeSlider); | ||
| this.highSlider = selectOrError<HTMLInputElement>("[name=high]", this.rangeSlider); | ||
| this.minLabel = selectOrError<HTMLSpanElement>(".text-start", this.rangeSlider); | ||
| this.maxLabel = selectOrError<HTMLSpanElement>(".text-end", this.rangeSlider); | ||
| this.rangeLabel = selectOrError<HTMLSpanElement>(".range-values", this.rangeSlider); | ||
| this.configurableMaxValue = maxValue; | ||
|
|
||
| const setValueFromNestedElements = (): void => { | ||
| this.value = { low: parseFloat(this.lowSlider.value), high: parseFloat(this.highSlider.value) }; | ||
|
|
@@ -48,7 +50,11 @@ export class RangeSlider { | |
| if (this.value.low > this.value.high) { | ||
| [this.value.low, this.value.high] = [this.value.high, this.value.low]; | ||
| } | ||
| this.rangeLabel.innerText = `${this.value.low} – ${this.value.high}`; | ||
|
|
||
| // Display "+" if high value equals the configurable max value | ||
| const highDisplay = | ||
| this.value.high === this.configurableMaxValue ? `${this.value.high}+` : this.value.high.toString(); | ||
| this.rangeLabel.innerText = `${this.value.low} – ${highDisplay}`; | ||
|
|
||
| // debounce on range change callback | ||
| if (this.debounceTimeout !== undefined) { | ||
|
|
@@ -61,11 +67,19 @@ export class RangeSlider { | |
|
|
||
| public onRangeChange(): void {} | ||
|
|
||
| public isOpenEnd(): boolean { | ||
| return this.configurableMaxValue !== undefined && this.value.high === this.configurableMaxValue; | ||
| } | ||
|
|
||
| public includeValues(values: number[]): void { | ||
| assert(Math.min(...values) >= this.allowed.low); | ||
| const max = Math.max(...values); | ||
| if (max > this.allowed.high) { | ||
| this.allowed.high = max; | ||
|
|
||
| // Use configurable max value if set, otherwise use the actual max from data | ||
| const effectiveMax = this.configurableMaxValue ?? max; | ||
|
|
||
| if (max > this.allowed.high || (this.configurableMaxValue && this.allowed.high !== this.configurableMaxValue)) { | ||
| this.allowed.high = effectiveMax; | ||
| this.updateNestedElements(); | ||
| this.reset(); | ||
| } | ||
|
|
@@ -81,6 +95,12 @@ export class RangeSlider { | |
| this.highSlider.min = this.allowed.low.toString(); | ||
| this.highSlider.max = this.allowed.high.toString(); | ||
| this.minLabel.innerText = this.allowed.low.toString(); | ||
| this.maxLabel.innerText = this.allowed.high.toString(); | ||
|
|
||
| // Display "+" in max label if configurable max value is set | ||
| const maxLabelDisplay = | ||
| this.configurableMaxValue && this.allowed.high === this.configurableMaxValue | ||
| ? `${this.allowed.high}+` | ||
| : this.allowed.high.toString(); | ||
| this.maxLabel.innerText = maxLabelDisplay; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be something like
escapejshere, I am not sure right now which method is the right one, but there is definitely one :D