Introduce editable table for creating score distributions #5723
Conversation
WalkthroughThis update introduces modifications to the database schema and the package dependencies. A new column named Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
app/assets/javascripts/components/input_table.ts (1)
91-121: Remove commented-out condition infirstUpdated.The
firstUpdatedmethod is well-structured, but consider removing the commented-out condition in the resize observer for clarity.- // if (Math.abs(parseInt(this.table.getWidth(2) as string) - this.descriptionColWidth) > 10) {Tools
GitHub Check: codecov/patch
[warning] 91-93: app/assets/javascripts/components/input_table.ts#L91-L93
Added lines #L91 - L93 were not covered by tests
[warning] 116-116: app/assets/javascripts/components/input_table.ts#L116
Added line #L116 was not covered by tests
[warning] 118-118: app/assets/javascripts/components/input_table.ts#L118
Added line #L118 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
config/locales/js/en.yml (2)
302-302: Enhance description help text with Markdown examplesConsider adding common Markdown examples to help users format their descriptions effectively.
- description_help: A description is optional. Markdown formatting can be used. This is visible to the students. + description_help: A description is optional. You can use Markdown formatting (e.g., **bold**, *italic*, bullet lists) to style the text. This is visible to the students.
310-315: Add help text for spreadsheet operationsThe PR objectives mention that users can prepare score items in advance using spreadsheet programs. Consider adding help text to guide users about this functionality.
jspreadsheet: + help: You can prepare score items in a spreadsheet program and paste them directly into this editor. Each row should contain name, description, maximum score, and visibility status. copy: Copy... deleteSelectedRows: Delete selected rows insertNewRowAfter: Insert new row after insertNewRowBefore: Insert new row before paste: Paste...config/locales/js/nl.yml (1)
307-309: Consider enhancing warning messages with specific guidance.While the warning messages are clear, they could be more helpful by including specific guidance:
- modified_warning: "Je hebt de maximumscore van een of meerdere scoreonderdelen aangepast. Alle afgewerkte evaluaties met dit scoreonderdeel zullen terug als onafgewerkt gemarkeerd worden." + modified_warning: "Je hebt de maximumscore van een of meerdere scoreonderdelen aangepast. Alle afgewerkte evaluaties met dit scoreonderdeel zullen terug als onafgewerkt gemarkeerd worden. Je zal deze opnieuw moeten evalueren." - deleted_warning: "Je hebt een of meerdere scoreonderdelen verwijderd. Dit zal ook de bijhorende scores van de studenten verwijderen." + deleted_warning: "Je hebt een of meerdere scoreonderdelen verwijderd. Dit zal ook de bijhorende scores van de studenten verwijderen. Deze actie kan niet ongedaan gemaakt worden." - validation_warning: "Alle scoreonderdelen moeten een naam en een maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000." + validation_warning: "Alle scoreonderdelen moeten een naam en een maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000. Controleer of alle velden correct zijn ingevuld."app/assets/javascripts/i18n/translations.json (1)
849-869: Verify translation consistencyThe Dutch translations are generally consistent with English, but there are some minor differences in formatting that should be standardized:
- Line 855: "Kopiëer" should be "Kopieer" (Dutch spelling)
- Line 866: The validation warning text structure differs slightly from English version
Apply these changes:
- "copy": "Kopiëer...", + "copy": "Kopieer...", - "validation_warning": "Alle scoreonderdelen moeten een naam en een maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000.", + "validation_warning": "Alle scoreonderdelen moeten een naam en een maximumscore hebben, en de maximumscore moet een getal zijn tussen 0 en 1000.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/assets/javascripts/i18n/translations.json(2 hunks)config/locales/js/en.yml(1 hunks)config/locales/js/nl.yml(1 hunks)
🔇 Additional comments (5)
config/locales/js/nl.yml (3)
298-301: LGTM!
The translations for basic score item fields are clear and follow common Dutch UI terminology.
302-303: LGTM!
The help texts for description and visibility are clear and informative.
305-306: LGTM!
The translations for action buttons follow standard Dutch UI conventions.
app/assets/javascripts/i18n/translations.json (2)
346-346: Update maximum_help text to match actual behavior
The help text still mentions increments of 0.25, but according to PR comments, the system allows steps of 0.01.
862-862: Update Dutch maximum_help text to match English changes
The Dutch translation has the same issue with the maximum_help text.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
app/assets/javascripts/components/input_table.ts (2)
84-84: Consider using a more robust number parsing approach.The current comma-to-dot replacement might not handle all number format cases correctly (e.g., multiple commas).
- maximum: (row[3] as string).replace(",", "."), // replace comma with dot for float representation + maximum: (row[3] as string).replace(/,/g, ".").replace(/\.(?=.*\.)/g, ""), // handle multiple separators
241-255: Consider using a more structured warning system.The current warning system concatenates strings and shows all warnings at once, which might be overwhelming for users.
+ interface Warning { + message: string; + type: 'deletion' | 'modification'; + } + confirmWarnings(): boolean { const old = this.scoreItems; const edited = this.editedScoreItems; const removed = old.some(item => !edited.some(e => e.id === item.id)); const maxEdited = old.some(item => edited.some(e => e.id === item.id && e.maximum !== item.maximum)); - let warnings = ""; + const warnings: Warning[] = []; if (removed) { - warnings += i18n.t("js.score_items.deleted_warning") + "\n"; + warnings.push({ message: i18n.t("js.score_items.deleted_warning"), type: 'deletion' }); } if (maxEdited) { - warnings += i18n.t("js.score_items.modified_warning") + "\n"; + warnings.push({ message: i18n.t("js.score_items.modified_warning"), type: 'modification' }); } - return warnings === "" || confirm(warnings); + return warnings.length === 0 || confirm(warnings.map(w => `${w.message}`).join('\n')); }config/locales/js/en.yml (1)
310-316: Add keyboard shortcuts and accessibility translationsConsider adding the following translations to improve usability:
- Keyboard shortcuts (mentioned in PR comments about browser differences)
- Accessibility labels for the table interface
jspreadsheet: copy: Copy... deleteRow: Delete row deleteSelectedRows: Delete selected rows insertNewRowAfter: Insert new row after insertNewRowBefore: Insert new row before paste: Paste... + shortcuts: + copy: Press Ctrl+C to copy + paste: Press Ctrl+V to paste, or use right-click menu + accessibility: + table: Score items table editor + cell: Score item cell + row: Score item rowconfig/locales/js/nl.yml (1)
307-309: Consider enhancing warning messages with specific guidanceWhile the warning messages are clear, they could be more helpful by including specific guidance:
- modified_warning: "Je hebt de maximumscore van een of meerdere scoreonderdelen aangepast. Alle afgewerkte evaluaties met dit scoreonderdeel zullen terug als onafgewerkt gemarkeerd worden." + modified_warning: "Je hebt de maximumscore van een of meerdere scoreonderdelen aangepast. Alle afgewerkte evaluaties met dit scoreonderdeel zullen terug als onafgewerkt gemarkeerd worden. Controleer en herbereken deze evaluaties." - deleted_warning: "Je hebt een of meerdere scoreonderdelen verwijderd. Dit zal ook de bijhorende scores van de studenten verwijderen." + deleted_warning: "Je hebt een of meerdere scoreonderdelen verwijderd. Dit zal ook de bijhorende scores van de studenten verwijderen. Deze actie kan niet ongedaan gemaakt worden."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/assets/javascripts/components/input_table.ts(1 hunks)app/assets/javascripts/i18n/translations.json(2 hunks)app/assets/stylesheets/models/score_items.css.scss(2 hunks)app/views/score_items/_exercise.html.erb(4 hunks)config/locales/js/en.yml(1 hunks)config/locales/js/nl.yml(1 hunks)config/locales/views/score_items/en.yml(1 hunks)config/locales/views/score_items/nl.yml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/assets/javascripts/components/input_table.ts
[warning] 1-8: app/assets/javascripts/components/input_table.ts#L1-L8
Added lines #L1 - L8 were not covered by tests
[warning] 22-22: app/assets/javascripts/components/input_table.ts#L22
Added line #L22 was not covered by tests
[warning] 37-37: app/assets/javascripts/components/input_table.ts#L37
Added line #L37 was not covered by tests
[warning] 39-39: app/assets/javascripts/components/input_table.ts#L39
Added line #L39 was not covered by tests
[warning] 41-41: app/assets/javascripts/components/input_table.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: app/assets/javascripts/components/input_table.ts#L43
Added line #L43 was not covered by tests
[warning] 47-47: app/assets/javascripts/components/input_table.ts#L47
Added line #L47 was not covered by tests
[warning] 49-50: app/assets/javascripts/components/input_table.ts#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-53: app/assets/javascripts/components/input_table.ts#L53
Added line #L53 was not covered by tests
[warning] 55-55: app/assets/javascripts/components/input_table.ts#L55
Added line #L55 was not covered by tests
[warning] 59-60: app/assets/javascripts/components/input_table.ts#L59-L60
Added lines #L59 - L60 were not covered by tests
[warning] 63-65: app/assets/javascripts/components/input_table.ts#L63-L65
Added lines #L63 - L65 were not covered by tests
[warning] 76-77: app/assets/javascripts/components/input_table.ts#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 79-80: app/assets/javascripts/components/input_table.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 94-96: app/assets/javascripts/components/input_table.ts#L94-L96
Added lines #L94 - L96 were not covered by tests
[warning] 99-100: app/assets/javascripts/components/input_table.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 104-104: app/assets/javascripts/components/input_table.ts#L104
Added line #L104 was not covered by tests
[warning] 107-108: app/assets/javascripts/components/input_table.ts#L107-L108
Added lines #L107 - L108 were not covered by tests
[warning] 111-113: app/assets/javascripts/components/input_table.ts#L111-L113
Added lines #L111 - L113 were not covered by tests
[warning] 115-118: app/assets/javascripts/components/input_table.ts#L115-L118
Added lines #L115 - L118 were not covered by tests
[warning] 122-122: app/assets/javascripts/components/input_table.ts#L122
Added line #L122 was not covered by tests
[warning] 124-126: app/assets/javascripts/components/input_table.ts#L124-L126
Added lines #L124 - L126 were not covered by tests
[warning] 128-130: app/assets/javascripts/components/input_table.ts#L128-L130
Added lines #L128 - L130 were not covered by tests
[warning] 132-133: app/assets/javascripts/components/input_table.ts#L132-L133
Added lines #L132 - L133 were not covered by tests
[warning] 135-135: app/assets/javascripts/components/input_table.ts#L135
Added line #L135 was not covered by tests
[warning] 137-137: app/assets/javascripts/components/input_table.ts#L137
Added line #L137 was not covered by tests
[warning] 142-143: app/assets/javascripts/components/input_table.ts#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 150-150: app/assets/javascripts/components/input_table.ts#L150
Added line #L150 was not covered by tests
[warning] 155-155: app/assets/javascripts/components/input_table.ts#L155
Added line #L155 was not covered by tests
[warning] 157-157: app/assets/javascripts/components/input_table.ts#L157
Added line #L157 was not covered by tests
🪛 Biome
app/assets/javascripts/components/input_table.ts
[error] 277-277: 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)
🔇 Additional comments (19)
app/assets/stylesheets/models/score_items.css.scss (2)
8-8: LGTM: Cursor style enhances spreadsheet-like experience
The text cursor style appropriately indicates the editable nature of the table cells, aligning with the Excel-like interface mentioned in the PR objectives.
94-102: Verify accessibility of disabled button
The styles effectively disable the last cell's button both visually and functionally. However, please verify:
- The purpose of disabling this specific button
- Whether users need to understand why this button is disabled
- If this meets accessibility guidelines for disabled elements
Consider adding an aria-disabled attribute and a tooltip explaining why the button is disabled.
✅ Verification successful
From the code context, I can see this is a score items input table using jspreadsheet-ce (previously jexcel) where the last cell contains a delete row button. Let me search for more specific context about the delete functionality.
Based on the code context, I can now provide a complete response about the disabled button in the last cell.
Accessibility considerations for disabled delete button are appropriate
The last row's delete button is intentionally disabled to ensure at least one score item remains in the evaluation table, which is a critical requirement as evidenced by:
- The code shows score items are essential for grading:
"nothing": "There are no score items for this exercise yet. You can only grade exercises with at least one score item." - The disabled state is appropriately conveyed through both:
- Visual feedback (opacity: 0.38)
- Functional feedback (pointer-events: none)
- The button already includes accessibility features:
- Clear purpose through title attribute:
title="${i18n.t("js.score_items.jspreadsheet.deleteRow")}" - Icon with descriptive class:
mdi-delete
- Clear purpose through title attribute:
The current implementation follows accessibility best practices by maintaining one required row and clearly communicating the disabled state both visually and programmatically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related accessibility attributes or tooltips in the component
rg -t ts -t js "aria-disabled|title=" --glob "**/*input_table*"
# Look for documentation about this disabled button
rg -t md -t txt "last.*button|disabled.*button" --glob "**/docs/*"
Length of output: 403
Script:
#!/bin/bash
# Search for related code in input table components and score items
rg -t ts -t js "class.*InputTable|function.*InputTable" -A 10
# Look for jexcel/jspreadsheet related code and documentation
rg -t ts -t js "jexcel|jspreadsheet" -A 5
# Search for any comments or documentation about the last cell or button
rg "last.*cell|last.*button" -g "!*.{css,scss}"
Length of output: 4283
Script:
#!/bin/bash
# Search for delete row related code and comments
rg "deleteRow|deleteCellRow" -A 10
# Look for score item validation or constraints that might explain why the last button is disabled
rg "ScoreItem.*validate|minimum.*score|maximum.*score" -A 5
# Search for any comments about requiring at least one row
rg -C 5 "at least one|minimum.*row"
Length of output: 44943
config/locales/views/score_items/en.yml (2)
39-40: Verify text alignment with new spreadsheet interface
The simplified text strings align with the new spreadsheet-like interface. However, consider whether "Add score items" provides enough context for users who are new to the system.
Consider adding tooltips or contextual help in the UI to compensate for the removed help texts, especially for first-time users. This could be implemented through the jspreadsheet-ce interface.
Line range hint 1-40: Consider impact of removing detailed help texts
While simplifying the UI is generally good, the removal of detailed help texts (description-help_html, visible-help, modify-help, score-help) might impact user understanding, especially for:
- Understanding score visibility rules
- Proper score item configuration
- Modification implications
Consider:
- Moving the removed help content to a dedicated documentation page
- Adding a help icon/link in the spreadsheet interface that points to this documentation
- Implementing progressive disclosure of help text for new users
Would you like me to help draft the documentation page content based on the removed help texts?
config/locales/views/score_items/nl.yml (1)
39-40: Verify translations for spreadsheet features.
Let's ensure all new spreadsheet-related features have corresponding translations.
✅ Verification successful
Let me gather more specific information about the spreadsheet-related translations and their usage.
This will help us verify if all the spreadsheet features used in the components have corresponding translations.
Let me check the actual implementations of the table components since the previous paths were incorrect.
This will help us locate and examine the actual table component implementations to verify if all their features have corresponding translations.
Let me check the JavaScript translations to ensure all spreadsheet-related strings are properly translated.
This will help us verify if all the spreadsheet-specific strings used in the input_table component (like copy, paste, delete row, etc.) have corresponding translations.
All spreadsheet features are properly translated
After analyzing the code and translations, I can confirm that all spreadsheet-related features used in the ScoreItemInputTable component have corresponding translations in both English and Dutch. The translations include:
- Basic table operations (copy, paste)
- Row management (insert row before/after, delete row, delete selected rows)
- Column headers and tooltips
- Table-specific messages and validations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for missing translations related to the spreadsheet interface
# Look for spreadsheet-related strings in the JavaScript files
echo "Searching for potential untranslated strings in JavaScript files..."
rg -i "jspreadsheet|spreadsheet|table|column|row|cell|copy|paste|reorder" app/javascript/
# Compare with available translations
echo "Checking available translations..."
rg -i "jspreadsheet|spreadsheet|table|column|row|cell|copy|paste|reorder" config/locales/views/score_items/
Length of output: 1631
Script:
#!/bin/bash
# Check the actual content of the translation files and related components
# Check the translation files content
echo "=== Translation files content ==="
cat config/locales/views/score_items/en.yml
cat config/locales/views/score_items/nl.yml
# Check the input table component implementation
echo -e "\n=== Input table component ==="
cat app/javascript/components/input_table.ts
# Check question table implementation
echo -e "\n=== Question table implementation ==="
cat app/javascript/question_table.ts
Length of output: 4618
Script:
#!/bin/bash
# Find and examine the table-related components
echo "=== Finding table-related files ==="
fd -e ts -e js table
fd -e ts -e js Table
echo -e "\n=== Examining table implementations ==="
for file in $(fd -e ts -e js table); do
echo "=== Content of $file ==="
cat "$file"
done
for file in $(fd -e ts -e js Table); do
echo "=== Content of $file ==="
cat "$file"
done
Length of output: 17433
Script:
#!/bin/bash
# Check JavaScript translations
echo "=== Checking JavaScript translations ==="
fd -e yml js.yml config/locales/
fd -e yml score_items config/locales/ -x cat {}
# Check for specific jspreadsheet strings in translations
echo -e "\n=== Checking specific spreadsheet strings ==="
rg -i "jspreadsheet|copy|paste|insert.*row|delete.*row" config/locales/
Length of output: 6016
app/assets/javascripts/components/input_table.ts (4)
1-25: LGTM! Well-structured type definitions and utilities.
The type definitions are clear and the utility function is properly typed. The code follows TypeScript best practices.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1-8: app/assets/javascripts/components/input_table.ts#L1-L8
Added lines #L1 - L8 were not covered by tests
[warning] 22-22: app/assets/javascripts/components/input_table.ts#L22
Added line #L22 was not covered by tests
203-205:
Clean up ResizeObserver on component disconnection.
The ResizeObserver should be cleaned up when the component is removed to prevent memory leaks.
+ private resizeObserver: ResizeObserver;
+
- new ResizeObserver(() => {
+ this.resizeObserver = new ResizeObserver(() => {
this.table.setWidth(2, this.descriptionColWidth);
}).observe(this.tableRef.value);
+ disconnectedCallback(): void {
+ super.disconnectedCallback();
+ this.resizeObserver?.disconnect();
+ }Likely invalid or redundant comment.
229-230: 🛠️ Refactor suggestion
Improve number validation regex.
The current regex doesn't handle decimal numbers less than 1 (e.g., 0.5).
- if (! /^\d{1,3}(\.\d*)?$/.test(item.maximum) || parseFloat(item.maximum) <= 0) {
+ if (! /^(?:\d{1,3}|\d*\.\d+)$/.test(item.maximum) || parseFloat(item.maximum) <= 0) {Likely invalid or redundant comment.
189-195:
Prevent memory leaks from Bootstrap tooltips.
The tooltips are initialized but never destroyed, which could lead to memory leaks.
+ private tooltips: Tooltip[] = [];
+
// init tooltips
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);
+ this.tooltips.push(new Tooltip(td));
}
});
+ disconnectedCallback(): void {
+ super.disconnectedCallback();
+ this.tooltips.forEach(tooltip => tooltip.dispose());
+ this.tooltips = [];
+ }Likely invalid or redundant comment.
config/locales/js/nl.yml (5)
298-301: LGTM: Basic field labels are clear and concise
The translations for the basic score item properties are appropriate and follow common Dutch UI terminology.
302-303: LGTM: Help texts are informative and clear
The help texts for description and visibility provide clear guidance in proper Dutch.
304-304: Skip: Maximum score help text issue already reported
305-306: LGTM: Action button labels are standard
The translations use appropriate standard Dutch terms for save and cancel actions.
310-316: Skip: Spreadsheet action text issues already reported
app/assets/javascripts/i18n/translations.json (2)
347-347: Update maximum_help text to match actual behavior.
The text about 0.25 increments needs clarification as discussed in PR comments.
864-864: Update Dutch maximum_help text to match English changes.
The Dutch translation should be updated to match the clarified English text.
app/views/score_items/_exercise.html.erb (3)
6-11: LGTM!
The heading structure with the edit button is well-integrated and follows proper Bootstrap conventions.
78-78: LGTM!
The initialization of the edit button in JavaScript is correctly implemented.
57-63: Ensure Bootstrap tooltips are initialized
Verify that Bootstrap tooltips are properly initialized so the tooltip on the visibility switch functions as expected.
Run the following script to check for tooltip initialization:
✅ Verification successful
Bootstrap tooltips are properly initialized
Based on the codebase analysis, tooltips are correctly initialized through the initTooltips function in utilities.ts, which is called:
- On page load via
ready.then(initTooltips)in bothframe.jsandapplication_pack.js - After dynamic content updates in various components
The initialization code properly:
- Cleans up any dead tooltips
- Initializes new tooltips on all elements with
data-bs-toggle="tooltip" - Handles tooltip content correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Bootstrap tooltips are initialized in the JavaScript code.
# Search for tooltip initialization in JavaScript files
rg 'bootstrap.Tooltip' --type js
Length of output: 32
Script:
#!/bin/bash
# Let's try different search patterns to find tooltip initialization
# Search for tooltip-related code with broader patterns
rg -i "tooltip" --type js -A 3
# Also search for data-bs-toggle initialization patterns
rg -i "data-bs-toggle" --type js -A 3
# Search for general Bootstrap initialization
rg -i "bootstrap.*initialize|initializeBootstrap" --type js -A 3
Length of output: 3020
Script:
#!/bin/bash
# Let's check the implementation of initTooltips in utilities.ts
rg -p "initTooltips" app/javascript/utilities.ts -A 10
# Also check if there are any other tooltip-related functions in utilities.ts
rg -p "tooltip" app/javascript/utilities.ts -A 5
Length of output: 239
Script:
#!/bin/bash
# Let's find the utilities.ts file first
fd utilities.ts
# Also search for any TypeScript files containing tooltip initialization
rg -p "initTooltips" --type ts -A 10
# And search for tooltip-related code in TypeScript files
rg -p "tooltip" --type ts -A 5
Length of output: 39158
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
Copilot reviewed 16 out of 31 changed files in this pull request and generated 2 suggestions.
Files not reviewed (15)
- app/assets/javascripts/i18n/translations.json: Language not supported
- app/assets/stylesheets/base.css.scss: Language not supported
- app/assets/stylesheets/models/score_items.css.scss: Language not supported
- app/assets/stylesheets/theme/jspreadsheet.css.scss: Language not supported
- app/views/score_items/index.js.erb: Evaluated as low risk
- app/views/score_items/_form.html.erb: Evaluated as low risk
- app/views/score_items/add_all.js.erb: Evaluated as low risk
- app/views/evaluations/_score_items.html.erb: Evaluated as low risk
- app/views/score_items/_copy.html.erb: Evaluated as low risk
- app/views/score_items/_exercise.html.erb: Evaluated as low risk
- app/models/score_item.rb: Evaluated as low risk
- app/controllers/score_items_controller.rb: Evaluated as low risk
- app/assets/javascripts/utilities.ts: Evaluated as low risk
- config/locales/views/score_items/en.yml: Evaluated as low risk
- app/javascript/packs/score_item.js: Evaluated as low risk
Comments skipped due to low confidence (4)
app/assets/javascripts/components/input_table.ts:15
- The 'maximum' field should be a number to avoid potential issues with numerical operations.
maximum: string;
app/assets/javascripts/components/input_table.ts:96
- Directly manipulating the DOM can lead to issues. Use the table instance's API to delete rows.
this.table.deleteRow(row.rowIndex-1);
app/assets/javascripts/score_item.ts:53
- Ensure that the editBtn element is present in the DOM before proceeding with the event listener. Add a check to handle cases where the element is not found.
const editBtn = element.querySelector(".edit-btn") as HTMLAnchorElement;
app/assets/javascripts/score_item.ts:57
- Ensure that the form element is present in the DOM before proceeding with the event listener. Add a check to handle cases where the element is not found.
const form = element.querySelector("d-score-item-input-table") as ScoreItemInputTable;
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/assets/javascripts/components/input_table.ts (1)
84-84: Consider using Number.parseFloat for consistent number parsing.Replace string manipulation with a more robust number parsing approach.
- maximum: (row[3] as string).replace(",", "."), // replace comma with dot for float representation + maximum: Number.parseFloat((row[3] as string).replace(",", ".")).toString(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/assets/javascripts/components/input_table.ts(1 hunks)
🧰 Additional context used
🪛 Biome
app/assets/javascripts/components/input_table.ts
[error] 277-277: 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)
🔇 Additional comments (3)
app/assets/javascripts/components/input_table.ts (3)
10-18: LGTM! Well-structured type definitions.
The ScoreItem type is well-defined with appropriate optional fields and type constraints.
203-205:
Prevent memory leaks from ResizeObserver.
The ResizeObserver should be properly disposed when the component is destroyed.
+ private resizeObserver: ResizeObserver;
+
// update description column width when the window is resized
- new ResizeObserver(() => {
+ this.resizeObserver = new ResizeObserver(() => {
this.table.setWidth(2, this.descriptionColWidth);
- }).observe(this.tableRef.value);
+ });
+ this.resizeObserver.observe(this.tableRef.value);
+
+ // Add to disconnectedCallback
+ disconnectedCallback(): void {
+ super.disconnectedCallback();
+ this.tooltips.forEach(tooltip => tooltip.dispose());
+ this.tooltips = [];
+ this.resizeObserver?.disconnect();
+ }Likely invalid or redundant comment.
189-195:
Prevent memory leaks from Bootstrap tooltips.
The tooltips should be properly disposed when the component is destroyed.
+ private tooltips: Tooltip[] = [];
+
// init tooltips
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);
+ this.tooltips.push(new Tooltip(td));
}
});
+
+ disconnectedCallback(): void {
+ super.disconnectedCallback();
+ this.tooltips.forEach(tooltip => tooltip.dispose());
+ this.tooltips = [];
+ }Likely invalid or redundant comment.
bmesuere
added
deploy naos
There was a problem hiding this comment.
Row removal is a lot clearer now 👍
A few more comments:
- the row remove button has the material mouseover circle, but this is a bit strange in the table which is style differently (and part of the circle is cut off)
- the visibility checkbox in the table is aligned badly (bottom-left)
- I apparently forgot to write down my comment on the max score visibility. The toggle there looks very strange
- I'm not totally convinced that swapping out the pencil for "edit" fixed the problem. To me, there's a disconnect between the button and the table however I have no clear suggestion on how to fix this.
- not changed in this PR but strange when I think about it: the bottom row now says maximum score but should this be total score?
Sorry for nitpicking!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/assets/javascripts/components/input_table.ts (2)
293-293: Review the necessity ofcontenteditable="true"on the table containerThe
<div>containing the table reference (this.tableRef) has the attributecontenteditable="true". Sincejspreadsheetmanages cell editing internally, settingcontenteditableon the container may not be necessary and could introduce unintended behavior or security issues. Consider removing this attribute if it's not required for the functionality.
246-252: Improve warning message display inconfirmWarnings()The warnings concatenated with
\nmay not render newlines properly in theconfirmdialog across all browsers, potentially leading to a less clear message for the user. To enhance user experience, consider using a custom modal dialog that properly formats multiline messages or employs line breaks that are consistently rendered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/assets/javascripts/components/input_table.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/assets/javascripts/components/input_table.ts
[error] 277-277: 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)
This pull request introduces jspreadsheet-ce as table editor to manage score items for an evaluation.
This way, copying, reusing and even initially filling it out all go a lot faster. The excel like layout should be easy to interact with for most users.
It also allows to prepare score items in advance in a spreadsheet program and pasting it here directly.
Reordering is partially supported. Doing when some solutions are already evaluated will force you to redo those evaluations.
Peek.2024-11-19.10-58.mp4
Darkmode is also supported
Closes #4940 and closes #2713