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

Introduce editable table for creating score distributions #5723

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
2ea4fd5
DIY first version
jorg-vr Aug 6, 2024
935ce06
Use jspreadsheet
jorg-vr Aug 6, 2024
0df802b
Disable unwanted options
jorg-vr Aug 7, 2024
afee968
Actually save updates
jorg-vr Aug 7, 2024
f6f7f81
Fix new rows
jorg-vr Aug 7, 2024
e45df51
Allow inserting in different order
jorg-vr Aug 7, 2024
94161d2
Add validation
jorg-vr Aug 7, 2024
6258c97
Delete no longer needed options
jorg-vr Aug 7, 2024
c1e43df
Fix toggle
jorg-vr Aug 7, 2024
e516a08
Introduce translations
jorg-vr Aug 8, 2024
4e8e4c9
Introduce translations
jorg-vr Aug 8, 2024
0ddc92a
Add help text
jorg-vr Aug 8, 2024
4d99358
Fix minor bugs
jorg-vr Aug 8, 2024
a52eeff
Show validation warning
jorg-vr Aug 8, 2024
2812c8d
Ask to confirm warnings
jorg-vr Aug 8, 2024
abadbe9
Improve buttons
jorg-vr Aug 8, 2024
2a0a4a8
Remove checkboxes
jorg-vr Aug 8, 2024
40d20f9
Fix row heights
jorg-vr Aug 8, 2024
99b27c8
Merge branch 'main' into feat/table-input
jorg-vr Aug 8, 2024
61ed540
Fix tests
jorg-vr Aug 8, 2024
66c5247
Remove system test
jorg-vr Aug 8, 2024
2451a67
Fix empty case
jorg-vr Aug 8, 2024
c68b4d0
Judge empty rows more kindly
jorg-vr Aug 8, 2024
ad5c0d8
Remove the export option
jorg-vr Aug 8, 2024
defc89e
Avoid type coercion
jorg-vr Aug 9, 2024
41d8fae
Remove comments
jorg-vr Aug 9, 2024
bf47ad7
Add docs
jorg-vr Aug 9, 2024
973371d
Add translations for menu
jorg-vr Aug 9, 2024
a7e363d
Fix dark mode
jorg-vr Aug 9, 2024
f1ac12c
Merge branch 'main' into feat/table-input
jorg-vr Aug 30, 2024
be2eec6
Merge branch 'main' into feat/table-input
jorg-vr Oct 25, 2024
838e3cc
Open edit table on click
jorg-vr Oct 25, 2024
88d9bb4
Align colums left as in non edit version
jorg-vr Oct 25, 2024
370b371
Remove outline of btn
jorg-vr Oct 25, 2024
cc50615
Change header text style
jorg-vr Oct 25, 2024
d443fa0
Improve number validation
jorg-vr Oct 25, 2024
5cb4711
Update warnings
jorg-vr Oct 25, 2024
56982a5
Fix paste in chrome
jorg-vr Oct 28, 2024
cc80dde
Update app/assets/javascripts/components/input_table.ts
jorg-vr Oct 28, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
244 changes: 244 additions & 0 deletions app/assets/javascripts/components/input_table.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
import { customElement, property } from "lit/decorators.js";
import { html, PropertyValues, TemplateResult } from "lit";
import jspreadsheet, { Column, JspreadsheetInstance } from "jspreadsheet-ce";
import { createRef, ref, Ref } from "lit/directives/ref.js";
import { DodonaElement } from "components/meta/dodona_element";
import { fetch, ready } from "utilities";
import { i18n } from "i18n/i18n";
import { Tooltip } from "bootstrap";

Check warning on line 8 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L1-L8

Added lines #L1 - L8 were not covered by tests

type CellData = string | number | boolean;
type ScoreItem = {
id: number | null;
name: string;
description?: string;
maximum: string;
visible: boolean;
order?: number;
}

type ColumnWithTooltip = Column & { tooltip?: string };

/**
* A spreadsheet table to edit score items.
*
* @element d-score-item-input-table
*
* @fires cancel - When the cancel button is clicked.
*
* @prop {string} route - The route to send the updated score items to.
* @prop {ScoreItem[]} scoreItems - The original score items, that will be displayed in the table.
*/
@customElement("d-score-item-input-table")
export class ScoreItemInputTable extends DodonaElement {

Check warning on line 33 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L33

Added line #L33 was not covered by tests
@property({ type: String })
route: string = "";

Check warning on line 35 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L35

Added line #L35 was not covered by tests
@property({ type: Array, attribute: "score-items" })
scoreItems: ScoreItem[] = [];

Check warning on line 37 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L37

Added line #L37 was not covered by tests

tableRef: Ref<HTMLDivElement> = createRef();

Check warning on line 39 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L39

Added line #L39 was not covered by tests
table: JspreadsheetInstance;

@property({ state: true })
hasErrors: boolean = false;

Check warning on line 43 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L43

Added line #L43 was not covered by tests

get tableWidth(): number {
return this.tableRef.value.clientWidth;

Check warning on line 46 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L45-L46

Added lines #L45 - L46 were not covered by tests
}

get descriptionColWidth(): number {

Check warning on line 49 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L49

Added line #L49 was not covered by tests
if (!this.tableRef.value) {
return 200;

Check warning on line 51 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L51

Added line #L51 was not covered by tests
}

// full width - borders - name column - maximum column - visible column - index column
const variableWidth = this.tableWidth - 14 - 200 - 100 - 100 - 50;
return Math.max(200, variableWidth);

Check warning on line 56 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L55-L56

Added lines #L55 - L56 were not covered by tests
}

get data(): CellData[][] {
return [
...this.scoreItems.map(item => [

Check warning on line 61 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L59-L61

Added lines #L59 - L61 were not covered by tests
item.id,
item.name,
item.description,
item.maximum,
item.visible
]),
["", "", "", "", false]
];
}

get editedScoreItems(): ScoreItem[] {
const tableData = this.table.getData();

Check warning on line 73 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L72-L73

Added lines #L72 - L73 were not covered by tests

const scoreItems = tableData.map((row: CellData[], index: number) => {
return {

Check warning on line 76 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L75-L76

Added lines #L75 - L76 were not covered by tests
id: row[0] as number | null,
name: row[1] as string,
description: row[2] as string,
maximum: (row[3] as string).replace(",", "."), // replace comma with dot for float representation
visible: row[4] as boolean,
order: index,
};
});

// filter out empty rows
return scoreItems.filter(item => !(item.name === "" && item.maximum === "" && item.description === "" && item.visible === false));
}

get columnConfig(): ColumnWithTooltip[] {
return [

Check warning on line 91 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L90-L91

Added lines #L90 - L91 were not covered by tests
{ type: "hidden", title: "id" },
{ type: "text", title: i18n.t("js.score_items.name"), width: 200, align: "left" },
{ type: "text", title: i18n.t("js.score_items.description"), width: this.descriptionColWidth, align: "left", tooltip: i18n.t("js.score_items.description_help") },
{ type: "numeric", title: i18n.t("js.score_items.maximum"), width: 100, align: "left", tooltip: i18n.t("js.score_items.maximum_help") },
{ type: "checkbox", title: i18n.t("js.score_items.visible"), width: 100, align: "left", tooltip: i18n.t("js.score_items.visible_help") },
];
}

async initTable(): Promise<void> {

Check warning on line 100 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L100

Added line #L100 was not covered by tests
// Wait for translations to be present
await ready;

Check warning on line 102 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L102

Added line #L102 was not covered by tests

this.table = jspreadsheet(this.tableRef.value, {

Check warning on line 104 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L104

Added line #L104 was not covered by tests
root: this,
data: this.data,
columns: this.columnConfig,
text: {
copy: i18n.t("js.score_items.jspreadsheet.copy"),
deleteSelectedRows: i18n.t("js.score_items.jspreadsheet.deleteSelectedRows"),
insertANewRowAfter: i18n.t("js.score_items.jspreadsheet.insertNewRowAfter"),
insertANewRowBefore: i18n.t("js.score_items.jspreadsheet.insertNewRowBefore"),
paste: i18n.t("js.score_items.jspreadsheet.paste"),
},
about: false,
allowDeleteColumn: false,
allowDeleteRow: true,
allowInsertColumn: false,
allowInsertRow: true,
allowManualInsertColumn: false,
allowManualInsertRow: true,
allowRenameColumn: false,
columnResize: false,
columnSorting: false,
minSpareRows: 1,
parseFormulas: false,
selectionCopy: false,
wordWrap: true,
defaultRowHeight: 30,
allowExport: false,
});

// init tooltips
this.columnConfig.forEach((column, index) => {
const td = this.tableRef.value.querySelector(`thead td[data-x="${index}"]`);

Check warning on line 135 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L134-L135

Added lines #L134 - L135 were not covered by tests
if (td && column.tooltip) {
td.setAttribute("title", column.tooltip);
new Tooltip(td);

Check warning on line 138 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L137-L138

Added lines #L137 - L138 were not covered by tests
}
Comment on lines +134 to +139
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Dispose of Tooltip instances to prevent memory leaks

The Tooltip instances created for column headers are not disposed of when the component is disconnected. This may lead to memory leaks. Consider storing references to the tooltips and disposing of them in the disconnectedCallback lifecycle method.

To address this, modify the code as follows:

 this.columnConfig.forEach((column, index) => {
     const td = this.tableRef.value.querySelector(`thead td[data-x="${index}"]`);
     if (td && column.tooltip) {
         td.setAttribute("title", column.tooltip);
-        new Tooltip(td);
+        const tooltip = new Tooltip(td);
+        this.tooltips.push(tooltip);
     }
 });
+
+disconnectedCallback(): void {
+    super.disconnectedCallback();
+    this.tooltips.forEach(tooltip => tooltip.dispose());
+    this.tooltips = [];
+}

And initialize tooltips in your class:

 export class ScoreItemInputTable extends DodonaElement {
+    tooltips: Tooltip[] = [];

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 134-135: app/assets/javascripts/components/input_table.ts#L134-L135
Added lines #L134 - L135 were not covered by tests


[warning] 137-138: app/assets/javascripts/components/input_table.ts#L137-L138
Added lines #L137 - L138 were not covered by tests

});


// update description column width when the window is resized
new ResizeObserver(() => {
this.table.setWidth(2, this.descriptionColWidth);

Check warning on line 145 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L144-L145

Added lines #L144 - L145 were not covered by tests
}).observe(this.tableRef.value);
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
}

protected firstUpdated(_changedProperties: PropertyValues): void {
super.firstUpdated(_changedProperties);

Check warning on line 150 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L149-L150

Added lines #L149 - L150 were not covered by tests

this.initTable();

Check warning on line 152 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L152

Added line #L152 was not covered by tests
}

validate(): boolean {

Check warning on line 155 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L155

Added line #L155 was not covered by tests
// Remove all error classes
this.tableRef.value.querySelectorAll("td.error").forEach(cell => {
cell.classList.remove("error");

Check warning on line 158 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L157-L158

Added lines #L157 - L158 were not covered by tests
});

const invalidCells: string[] = [];
const data = this.editedScoreItems;
data.forEach(item => {
const row = item.order + 1;

Check warning on line 164 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L161-L164

Added lines #L161 - L164 were not covered by tests
if (item.name === "") {
invalidCells.push("B" + row);

Check warning on line 166 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L166

Added line #L166 was not covered by tests
}
// Check if maximum is a positive number < 1000
// we use a regex instead of parseFloat because parseFloat is too lenient
if (! /^\d{1,3}(\.\d*)?$/.test(item.maximum) || parseFloat(item.maximum) <= 0) {
invalidCells.push("D" + row);

Check warning on line 171 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L171

Added line #L171 was not covered by tests
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
}
});
invalidCells.forEach(cell => {
this.table.getCell(cell).classList.add("error");

Check warning on line 175 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L174-L175

Added lines #L174 - L175 were not covered by tests
});
this.hasErrors = invalidCells.length > 0;
return !this.hasErrors;

Check warning on line 178 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L177-L178

Added lines #L177 - L178 were not covered by tests
}
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved

confirmWarnings(): boolean {
const old = this.scoreItems;
const edited = this.editedScoreItems;
const removed = old.some(item => !edited.some(e => e.id === item.id));

Check warning on line 184 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L181-L184

Added lines #L181 - L184 were not covered by tests
const maxEdited = old.some(item => edited.some(e => e.id === item.id && e.maximum !== item.maximum));

let warnings = "";

Check warning on line 187 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L187

Added line #L187 was not covered by tests
if (removed) {
warnings += i18n.t("js.score_items.deleted_warning") + "\n";

Check warning on line 189 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L189

Added line #L189 was not covered by tests
}
if (maxEdited) {
warnings += i18n.t("js.score_items.modified_warning") + "\n";

Check warning on line 192 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L192

Added line #L192 was not covered by tests
}

return warnings === "" || confirm(warnings);
}

async save(): Promise<void> {

Check warning on line 198 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L198

Added line #L198 was not covered by tests
if (!this.validate()) {
return;

Check warning on line 200 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L200

Added line #L200 was not covered by tests
}

if (!this.confirmWarnings()) {
return;

Check warning on line 204 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L204

Added line #L204 was not covered by tests
}

const response = await fetch(this.route, {

Check warning on line 207 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L207

Added line #L207 was not covered by tests
method: "PATCH",
headers: {
"Content-Type": "application/json"
},
body: JSON.stringify({
score_items: this.editedScoreItems
})
});
if (response.ok) {
const js = await response.text();
eval(js);

Check warning on line 218 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L217-L218

Added lines #L217 - L218 were not covered by tests
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
}
}
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +198 to +220
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using eval for executing response scripts

Using eval can introduce security risks and performance issues. Consider refactoring the code to avoid using eval, possibly by processing the JSON response directly or using a safer alternative.

Refactor the save method:

 if (response.ok) {
-    const js = await response.text();
-    eval(js);
+    const data = await response.json();
+    // Process the data as needed
 }

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Biome

[error] 218-218: eval() exposes to security risks and performance issues.

See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().

(lint/security/noGlobalEval)


cancel(): void {

Check warning on line 222 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L222

Added line #L222 was not covered by tests
if (this.table) {
this.table.setData(this.data);

Check warning on line 224 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L224

Added line #L224 was not covered by tests
}
this.dispatchEvent(new Event("cancel"));

Check warning on line 226 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L226

Added line #L226 was not covered by tests
}


render(): TemplateResult {
return html`

Check warning on line 231 in app/assets/javascripts/components/input_table.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/input_table.ts#L230-L231

Added lines #L230 - L231 were not covered by tests
${this.hasErrors ? html`<div class="alert alert-danger">${i18n.t("js.score_items.validation_warning")}</div>` : ""}
<div style="width: 100%" ${ref(this.tableRef)} contenteditable="true"></div>
<div class="d-flex justify-content-end">
<button @click=${this.cancel} class="btn btn-text me-1">
${i18n.t("js.score_items.cancel")}
</button>
<button @click=${this.save} class="btn btn-filled">
${i18n.t("js.score_items.save")}
</button>
</div>
`;
}
}
42 changes: 42 additions & 0 deletions app/assets/javascripts/i18n/translations.json
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,27 @@
"score_item": {
"error": "Error while updating"
},
"score_items": {
"cancel": "Cancel",
"deleted_warning": "You have deleted one or more score items. This will also delete all scores for these items.",
"description": "Description",
"description_help": "A description is optional. Markdown formatting can be used. This is visible to the students.",
"jspreadsheet": {
"copy": "Copy...",
"deleteSelectedRows": "Delete selected rows",
"insertNewRowAfter": "Insert new row after",
"insertNewRowBefore": "Insert new row before",
"paste": "Paste..."
},
"maximum": "Maximum",
"maximum_help": "The maximum grade for this score item. The grade should be between 0 and 1000, and works in increments of 0.25.",
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
"modified_warning": "You have changed the maximum score of one or more score items. This will mark all completed evaluations with this score item as uncompleted.",
"name": "Name",
"save": "Save",
"validation_warning": "All score items must have a name and a maximum score, and the maximum score must be between 0 and 1000.",
"visible": "Visible",
"visible_help": "Make the score item visible to students once the evaluation is released."
},
"sign_in_search_bar": {
"institution_search": "Type to search for your institution",
"log_in": "Sign in"
Expand Down Expand Up @@ -868,6 +889,27 @@
"score_item": {
"error": "Fout bij bijwerken"
},
"score_items": {
"cancel": "Annuleren",
"deleted_warning": "Je hebt een of meerdere scoreonderdelen verwijderd. Dit zal ook de bijhorende scores van de studenten verwijderen.",
"description": "Beschrijving",
"description_help": "Een beschrijving is optioneel en kan in Markdown geschreven worden. Dit is zichtbaar voor de studenten.",
"jspreadsheet": {
"copy": "Kopiëer...",
"deleteSelectedRows": "Verwijder geselecteerde rijen",
"insertNewRowAfter": "Voeg nieuwe rij toe na deze",
"insertNewRowBefore": "Voeg nieuwe rij toe voor deze",
"paste": "Plak..."
},
"maximum": "Maximum",
"maximum_help": "De maximumscore voor dit scoreonderdeel. Dit moet een getal zijn tussen 0 en 1000 en gaat in stappen van 0.25.",
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
"modified_warning": "Je hebt de maximumscore van een of meerdere scoreonderdelen aangepast. Alle afgewerkte evaluaties met dit scoreonderdeel zullen terug als onafgewerkt gemarkeerd worden.",
"name": "Naam",
"save": "Opslaan",
"validation_warning": "Alle scoreonderdelen moeten een naam en een maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000.",
"visible": "Zichtbaar",
"visible_help": "Maak het scoreonderdeel zichtbaar voor studenten eens de evaluatie vrijgegeven is."
},
"sign_in_search_bar": {
"institution_search": "Typ om jouw school te vinden",
"log_in": "Aanmelden"
Expand Down
35 changes: 23 additions & 12 deletions app/assets/javascripts/score_item.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { fetch } from "utilities";
import { i18n } from "i18n/i18n";
import { ScoreItemInputTable } from "components/input_table";

function commonCheckboxInit(
element: HTMLElement,
Expand Down Expand Up @@ -44,18 +45,28 @@
});
}

function initItemVisibilityCheckboxes(element: HTMLElement): void {
commonCheckboxInit(element, ".visibility-checkbox", checked => {
return {

score_item: {
visible: checked
}
};
});
}

export function initVisibilityCheckboxes(element: HTMLElement): void {
initTotalVisibilityCheckboxes(element);
initItemVisibilityCheckboxes(element);
}

export function initEditButton(element: HTMLElement): void {
const editBtn = element.querySelector(".edit-btn") as HTMLAnchorElement;
const table = element.querySelector(".score-items-table") as HTMLTableElement;
const form = element.querySelector("d-score-item-input-table") as ScoreItemInputTable;

Check warning on line 55 in app/assets/javascripts/score_item.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/score_item.ts#L52-L55

Added lines #L52 - L55 were not covered by tests

jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
const activateEditMode = (e: MouseEvent): void => {
e.preventDefault();
table.classList.add("d-none");
editBtn.classList.add("d-none");
form.classList.remove("d-none");

Check warning on line 61 in app/assets/javascripts/score_item.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/score_item.ts#L57-L61

Added lines #L57 - L61 were not covered by tests
};
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved

editBtn.addEventListener("click", activateEditMode);
table.addEventListener("click", activateEditMode);

Check warning on line 65 in app/assets/javascripts/score_item.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/score_item.ts#L64-L65

Added lines #L64 - L65 were not covered by tests
Comment on lines +64 to +65
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve click target specificity and accessibility.

The current implementation attaches a click handler to the entire table, which might interfere with other interactive elements within the table. Additionally, keyboard accessibility should be considered.

-    editBtn.addEventListener("click", activateEditMode);
-    table.addEventListener("click", activateEditMode);
+    // Add role and keyboard support
+    editBtn.setAttribute("role", "button");
+    editBtn.setAttribute("tabindex", "0");
+    editBtn.addEventListener("click", activateEditMode);
+    editBtn.addEventListener("keydown", (e: KeyboardEvent) => {
+        if (e.key === "Enter" || e.key === " ") {
+            e.preventDefault();
+            activateEditMode(e as unknown as MouseEvent);
+        }
+    });
+
+    // Add edit button to table header instead of entire table
+    const tableHeader = table.querySelector("thead");
+    if (tableHeader) {
+        tableHeader.addEventListener("click", activateEditMode);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
editBtn.addEventListener("click", activateEditMode);
table.addEventListener("click", activateEditMode);
// Add role and keyboard support
editBtn.setAttribute("role", "button");
editBtn.setAttribute("tabindex", "0");
editBtn.addEventListener("click", activateEditMode);
editBtn.addEventListener("keydown", (e: KeyboardEvent) => {
if (e.key === "Enter" || e.key === " ") {
e.preventDefault();
activateEditMode(e as unknown as MouseEvent);
}
});
// Add edit button to table header instead of entire table
const tableHeader = table.querySelector("thead");
if (tableHeader) {
tableHeader.addEventListener("click", activateEditMode);
}


form.addEventListener("cancel", () => {
table.classList.remove("d-none");
editBtn.classList.remove("d-none");
form.classList.add("d-none");

Check warning on line 70 in app/assets/javascripts/score_item.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/score_item.ts#L67-L70

Added lines #L67 - L70 were not covered by tests
});
}
2 changes: 1 addition & 1 deletion app/assets/javascripts/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,5 +269,5 @@ export {
ready,
getParentByClassName,
setHTMLExecuteScripts,
replaceHTMLExecuteScripts,
replaceHTMLExecuteScripts
};
5 changes: 5 additions & 0 deletions app/assets/stylesheets/base.css.scss
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,16 @@
@import "../../../node_modules/bootstrap/scss/offcanvas";
@import "../../../node_modules/bootstrap/scss/utilities/api";

// jspreadsheet styles
@import "../../../node_modules/jspreadsheet-ce/dist/jspreadsheet";
@import "../../../node_modules/jsuites/dist/jsuites";

:root {
--scrollbar-width: 20px;
}

// 5. Add additional custom code here
@import "theme/jspreadsheet.css.scss";
@import "bootstrap_style_overrides.css.scss";
@import "libraries.css.scss";
@import "material_icons.css.scss";
Expand Down
Loading
Loading