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
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
We allow steps of 0.01. But when giving grades, the number increase/decrease buttons take steps of 0.25. So It might be advised to also use steps of 0.25 for your max. I am open to removing the from the tooltip, enforcing the 0.25 or simply leaving as is. |
pdawyndt
changed the title
In Firefox everything works as expected In chrome
Update: this is fixed now |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
app/assets/javascripts/score_item.ts (1)
67-71: Consider adding transition animations.The sudden show/hide of elements might be jarring. Consider adding smooth transitions for better UX.
+ // Add CSS classes for transitions + table.classList.add("transition-opacity"); + editBtn.classList.add("transition-opacity"); + form.classList.add("transition-opacity"); + form.addEventListener("cancel", () => { table.classList.remove("d-none"); editBtn.classList.remove("d-none"); form.classList.add("d-none"); });Add these CSS classes to your stylesheets:
.transition-opacity { transition: opacity 0.2s ease-in-out; } .d-none { opacity: 0; pointer-events: none; }app/assets/stylesheets/theme/jspreadsheet.css.scss (2)
197-203: Consider enhancing the disabled state visual feedback.While the current implementation uses muted colors for disabled items, consider adding additional visual cues for better accessibility.
Add a subtle style to make the disabled state more apparent:
.jcontextmenu .jcontextmenu-disabled a { color: var(--d-on-surface-muted); + cursor: not-allowed; + opacity: 0.7; }
1-213: Consider adding documentation for theme customization.Since this implements a custom theme for jspreadsheet-ce, it would be helpful to add documentation comments explaining:
- The available CSS variables and their purposes
- How to customize the theme for different color schemes
- Any accessibility considerations for theme customization
This aligns with the PR objectives mentioning that documentation updates are pending.
Would you like me to help generate the documentation comments?
config/locales/js/en.yml (1)
321-326: Add browser-specific paste instructionsBased on the PR comments, there were issues with copy-paste functionality in Chrome that required workarounds. Consider adding browser-specific instructions to help users.
Consider adding these translations:
jspreadsheet: copy: Copy... deleteSelectedRows: Delete selected rows insertNewRowAfter: Insert new row after insertNewRowBefore: Insert new row before paste: Paste... + paste_help_chrome: In Chrome, right-click to paste or focus an input field before using Ctrl+V + paste_help_firefox: Use Ctrl+V to pasteconfig/locales/js/nl.yml (2)
318-320: LGTM: Warning messages are clear and informative.The warning messages effectively communicate the consequences of user actions. Consider a minor enhancement for the validation message:
- 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 geldige maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000."
321-326: Consider adding browser-specific copy-paste instructions.Given the reported copy-paste behavior differences between browsers, consider adding more detailed tooltips:
jspreadsheet: - copy: Kopiëer... + copy: Kopiëer... (Ctrl+C of rechtsklik) - paste: Plak... + paste: Plak... (Ctrl+V of rechtsklik)app/assets/javascripts/components/input_table.ts (3)
155-179: Consider adding unit tests for thevalidatemethodThe
validatemethod contains important validation logic that ensures data integrity. Adding unit tests for this method would help catch potential issues early and maintain code reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 155-155: app/assets/javascripts/components/input_table.ts#L155
Added line #L155 was not covered by tests
[warning] 157-158: app/assets/javascripts/components/input_table.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-164: app/assets/javascripts/components/input_table.ts#L161-L164
Added lines #L161 - L164 were not covered by tests
[warning] 166-166: app/assets/javascripts/components/input_table.ts#L166
Added line #L166 was not covered by tests
[warning] 171-171: app/assets/javascripts/components/input_table.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: app/assets/javascripts/components/input_table.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 177-178: app/assets/javascripts/components/input_table.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
80-80: Improve handling of decimal separators in themaximumfieldReplacing commas with dots may not be sufficient for all locales or input formats. Consider using localization libraries or more robust parsing methods to handle different decimal formats.
For example, you could use
parseFloatdirectly and handle potentialNaNresults appropriately.
116-121: Simplify configuration by settingallowManualInsertRowandallowManualInsertColumnSince
allowInsertRowandallowInsertColumnare already set totrueandfalserespectively, you may not need to setallowManualInsertRowandallowManualInsertColumnexplicitly, as they might default to the values ofallowInsertRowandallowInsertColumn.Review the necessity of these configurations to simplify the code.
📜 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/javascripts/score_item.ts (2 hunks)
- app/assets/javascripts/utilities.ts (1 hunks)
- app/assets/stylesheets/theme/jspreadsheet.css.scss (1 hunks)
- config/locales/js/en.yml (1 hunks)
- config/locales/js/nl.yml (1 hunks)
- test/javascript/utilities.test.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/javascript/utilities.test.js
🧰 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] 33-33: app/assets/javascripts/components/input_table.ts#L33
Added line #L33 was not covered by tests
[warning] 35-35: app/assets/javascripts/components/input_table.ts#L35
Added line #L35 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] 43-43: app/assets/javascripts/components/input_table.ts#L43
Added line #L43 was not covered by tests
[warning] 45-46: app/assets/javascripts/components/input_table.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 49-49: app/assets/javascripts/components/input_table.ts#L49
Added line #L49 was not covered by tests
[warning] 51-51: app/assets/javascripts/components/input_table.ts#L51
Added line #L51 was not covered by tests
[warning] 55-56: app/assets/javascripts/components/input_table.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 59-61: app/assets/javascripts/components/input_table.ts#L59-L61
Added lines #L59 - L61 were not covered by tests
[warning] 72-73: app/assets/javascripts/components/input_table.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-76: app/assets/javascripts/components/input_table.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 90-91: app/assets/javascripts/components/input_table.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 100-100: app/assets/javascripts/components/input_table.ts#L100
Added line #L100 was not covered by tests
[warning] 102-102: app/assets/javascripts/components/input_table.ts#L102
Added line #L102 was not covered by tests
[warning] 104-104: app/assets/javascripts/components/input_table.ts#L104
Added line #L104 was not covered by tests
[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
[warning] 144-145: app/assets/javascripts/components/input_table.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 149-150: app/assets/javascripts/components/input_table.ts#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-152: app/assets/javascripts/components/input_table.ts#L152
Added line #L152 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-158: app/assets/javascripts/components/input_table.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-164: app/assets/javascripts/components/input_table.ts#L161-L164
Added lines #L161 - L164 were not covered by tests
[warning] 166-166: app/assets/javascripts/components/input_table.ts#L166
Added line #L166 was not covered by tests
[warning] 171-171: app/assets/javascripts/components/input_table.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: app/assets/javascripts/components/input_table.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 177-178: app/assets/javascripts/components/input_table.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-184: app/assets/javascripts/components/input_table.ts#L181-L184
Added lines #L181 - L184 were not covered by tests
🪛 Biome
app/assets/javascripts/components/input_table.ts
[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)
🔇 Additional comments (18)
app/assets/javascripts/score_item.ts (2)
3-3: LGTM: Import statement is correctly structured.The import of ScoreItemInputTable aligns with the PR objective of introducing spreadsheet functionality.
49-50: Verify the removal of item visibility functionality.The removal of
initItemVisibilityCheckboxessuggests that item visibility controls have been moved to the new table editor. Let's verify this assumption.✅ Verification successful
Based on the search results, I can now generate a final response:
The removal of item visibility functionality is part of a deliberate refactoring
The visibility controls have been consolidated into the new table-based UI implementation. This is evidenced by:
- The presence of visibility column in
input_table.ts:{ type: "checkbox", title: i18n.t("js.score_items.visible")...}- The remaining
initTotalVisibilityCheckboxesfunction handles only total score visibility through a common checkbox initialization pattern- No references to the old
initItemVisibilityCheckboxesfunction exist in the codebase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to item visibility functionality rg -l "item.*visibility" --type ts # Search for visibility controls in the new table editor ast-grep --pattern 'class ScoreItemInputTable { $$$ visibility($$$) { $$$ } $$$ }'Length of output: 133
Script:
#!/bin/bash # Let's try a broader search for visibility-related code rg "visibility" --type ts -A 5 # Also search for any references to the old function name rg "initItemVisibilityCheckboxes" --type ts # Look for checkbox-related code in table components rg "checkbox" --type ts -A 5 # Search for potential table editor files fd "table.*\.(ts|tsx)$"Length of output: 27599
app/assets/stylesheets/theme/jspreadsheet.css.scss (8)
1-15: LGTM! Good use of CSS variables for theming.The root variable definition and container styles are well-structured and maintain consistency with the theme system.
30-48: LGTM! Well-structured header styles with clear state management.The header styles effectively handle different states (default, dragging, selected) while maintaining consistent typography.
69-71: Verify color contrast for readonly cells.The readonly state uses 30% opacity (
rgba(var(--d-on-surface-rgb), 0.3)). Please ensure this meets WCAG color contrast requirements for accessibility.Consider using a CSS custom property for the opacity value to make it easier to adjust if needed:
- color: rgba(var(--d-on-surface-rgb), 0.3); + --readonly-opacity: 0.3; + color: rgba(var(--d-on-surface-rgb), var(--readonly-opacity));
81-87: LGTM! Consistent footer styling.The footer styles maintain visual consistency with other table sections.
89-137: LGTM! Clear visual feedback for cell interactions.The highlight and selection styles effectively communicate the current state to users while maintaining visual consistency.
139-150: LGTM! Good editor visibility management.The editor styles ensure good visibility and usability of the rich text interface.
152-170: LGTM! Clear visual feedback for errors and copying.The styles effectively communicate error states and copy operations to users.
26-28: Consider the accessibility impact of disabling text selection.Preventing text selection (
::selection { background-color: transparent; }) might impact accessibility for users who rely on text selection for various purposes, such as using screen readers or copying content.Let's check if this is a requirement from the jspreadsheet library:
✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the context better and see if there's any documentation or code that explains why text selection is being disabled.
I want to verify if this selection behavior is part of the jspreadsheet library's default configuration or if it's a custom override. The previous results show other selection styles in the codebase that don't disable selection, suggesting this might be specific to the spreadsheet component.
Disabling text selection is consistent with spreadsheet functionality
The transparent selection style is intentional and appropriate in this context. The jspreadsheet configuration shows that this is a spreadsheet-like interface where:
- Custom selection behavior is managed by the library (
selectionCopy: false)- It's an input table component with editable cells
- The library provides its own selection mechanisms for spreadsheet operations
The disabled text selection doesn't impact accessibility as users can still:
- Edit cell contents normally
- Use keyboard navigation (standard spreadsheet behavior)
- Copy/paste through the spreadsheet's built-in mechanisms (evidenced by copy/paste i18n strings)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar selection styles in the jspreadsheet source rg -i "::selection.*transparent"Length of output: 34
Script:
#!/bin/bash # Let's search for any selection-related styles in the codebase rg -i "::selection" -A 2 # Also search for any comments or documentation about selection behavior rg -i "selection|selectable" --type md rg -i "selection|selectable" --type txt # Check if there are any JavaScript files that might control selection behavior rg -i "\.select|selectable|selection" --type js -A 2Length of output: 2302
Script:
#!/bin/bash # Let's specifically look for jspreadsheet/jexcel related files and configurations rg -i "jspreadsheet|jexcel" --type js -A 3 # Also search for any documentation about jspreadsheet configuration fd -e md -e txt -H -I | xargs rg -i "jspreadsheet|jexcel" # Look for any JavaScript code that might initialize or configure jspreadsheet ast-grep --pattern 'new Jspreadsheet($$$)' ast-grep --pattern 'jspreadsheet($$$)'Length of output: 2837
app/assets/javascripts/utilities.ts (1)
272-272: LGTM! The export addition is well-aligned with the PR objectives.The
replaceHTMLExecuteScriptsfunction is a necessary export for supporting dynamic content updates in the new table editor implementation. The function is well-implemented with proper security considerations using DOMParser.Let's verify the usage of this newly exported function:
✅ Verification successful
The exported function is correctly used in view templates for dynamic content updates
The verification shows that
replaceHTMLExecuteScriptsis:
- Imported in the main application pack
- Used in score-related views (
scores/show.js.erb) for updating score form wrappers- Used in series views (
series/show.js.erb) for updating series cards- Implementation in
utilities.tsconfirms secure DOM manipulation using DOMParser🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of replaceHTMLExecuteScripts in the codebase # Expected: Usage in table-related components, particularly with jspreadsheet-ce integration # Search for imports of replaceHTMLExecuteScripts echo "Checking imports:" rg "import.*replaceHTMLExecuteScripts.*from.*utilities" -A 2 # Search for usage of the function echo "Checking usage:" rg "replaceHTMLExecuteScripts\(" -A 2Length of output: 1464
config/locales/js/en.yml (2)
309-312: LGTM! Basic field labels are clear and concise.
319-320: LGTM! Standard action button labels.config/locales/js/nl.yml (2)
309-312: LGTM: Core field translations are accurate and complete.The Dutch translations for the basic score item fields are clear and appropriate.
316-317: LGTM: Action button translations are consistent.The translations for save and cancel actions align with existing patterns in the file.
app/assets/javascripts/i18n/translations.json (2)
366-386: LGTM! Translations are consistent and completeThe translations between English and Dutch are well-maintained, with appropriate cultural adaptations while preserving the intended meaning.
Also applies to: 892-912
371-377: LGTM! Translations support all required featuresThe translations properly support:
- Copy/paste functionality for score items
- Row management (insert/delete)
- All necessary UI elements for the jspreadsheet integration
Also applies to: 897-903
app/assets/javascripts/components/input_table.ts (1)
198-220: Avoid usingevaldue to security and performance concernsUsing
evalcan introduce security risks and performance issues. It's recommended to refactor the code to avoid usingevalby directly handling the response or using safer alternatives.🧰 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)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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
🪛 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] 33-33: app/assets/javascripts/components/input_table.ts#L33
Added line #L33 was not covered by tests
[warning] 35-35: app/assets/javascripts/components/input_table.ts#L35
Added line #L35 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] 43-43: app/assets/javascripts/components/input_table.ts#L43
Added line #L43 was not covered by tests
[warning] 45-46: app/assets/javascripts/components/input_table.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 49-49: app/assets/javascripts/components/input_table.ts#L49
Added line #L49 was not covered by tests
[warning] 51-51: app/assets/javascripts/components/input_table.ts#L51
Added line #L51 was not covered by tests
[warning] 55-56: app/assets/javascripts/components/input_table.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 59-61: app/assets/javascripts/components/input_table.ts#L59-L61
Added lines #L59 - L61 were not covered by tests
[warning] 72-73: app/assets/javascripts/components/input_table.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-76: app/assets/javascripts/components/input_table.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 90-91: app/assets/javascripts/components/input_table.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 100-100: app/assets/javascripts/components/input_table.ts#L100
Added line #L100 was not covered by tests
[warning] 102-102: app/assets/javascripts/components/input_table.ts#L102
Added line #L102 was not covered by tests
[warning] 104-104: app/assets/javascripts/components/input_table.ts#L104
Added line #L104 was not covered by tests
[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
[warning] 144-145: app/assets/javascripts/components/input_table.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 149-150: app/assets/javascripts/components/input_table.ts#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-152: app/assets/javascripts/components/input_table.ts#L152
Added line #L152 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-158: app/assets/javascripts/components/input_table.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-164: app/assets/javascripts/components/input_table.ts#L161-L164
Added lines #L161 - L164 were not covered by tests
[warning] 166-166: app/assets/javascripts/components/input_table.ts#L166
Added line #L166 was not covered by tests
[warning] 171-171: app/assets/javascripts/components/input_table.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: app/assets/javascripts/components/input_table.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 177-178: app/assets/javascripts/components/input_table.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-184: app/assets/javascripts/components/input_table.ts#L181-L184
Added lines #L181 - L184 were not covered by tests
🪛 Biome
app/assets/javascripts/components/input_table.ts
[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)
🔇 Additional comments (1)
app/assets/javascripts/components/input_table.ts (1)
198-220:⚠️ Potential issueHandle non-OK HTTP responses in the
savemethodCurrently, the
savemethod only handles successful responses (response.ok). It's advisable to handle non-OK responses to provide feedback to the user in case of errors, such as network issues or server errors.Add error handling for failed requests:
if (response.ok) { const js = await response.text(); eval(js); +} else { + const errorMessage = await response.text(); + alert(`Save failed: ${errorMessage}`); }Likely invalid or redundant comment.
🧰 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)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/assets/javascripts/score_item.ts (2)
70-74: Add error handling and cleanup for the cancel event.The cancel event handler should include error handling and proper cleanup of resources.
- form.addEventListener("cancel", () => { + const handleCancel = () => { table.classList.remove("d-none"); editBtn.classList.remove("d-none"); form.classList.add("d-none"); - }); + }; + + form.addEventListener("cancel", handleCancel); + + // Cleanup function for removing event listeners + return () => { + form.removeEventListener("cancel", handleCancel); + element.removeEventListener("click", handleClick); + };
52-58: Consider type safety improvements for DOM queries.While past reviews addressed null checks, we should also ensure proper type narrowing and custom type guards.
+interface ScoreItemElements { + editBtn: HTMLAnchorElement; + table: HTMLTableElement; + rows: NodeListOf<HTMLTableRowElement>; + header: HTMLTableSectionElement; + form: ScoreItemInputTable; +} + +function isScoreItemElements(elements: Partial<ScoreItemElements>): elements is ScoreItemElements { + return !!(elements.editBtn && elements.table && elements.rows && + elements.header && elements.form); +} + export function initEditButton(element: HTMLElement): void { - const editBtn = element.querySelector(".edit-btn") as HTMLAnchorElement; - const table = element.querySelector(".score-items-table") as HTMLTableElement; - const allRowsButLast = table.querySelectorAll("tr:not(:last-child)"); - const tableHeader = table.querySelector("thead") as HTMLTableSectionElement; - const form = element.querySelector("d-score-item-input-table") as ScoreItemInputTable; + const elements = { + editBtn: element.querySelector<HTMLAnchorElement>(".edit-btn"), + table: element.querySelector<HTMLTableElement>(".score-items-table"), + rows: element.querySelectorAll<HTMLTableRowElement>("tr:not(:last-child)"), + header: element.querySelector<HTMLTableSectionElement>("thead"), + form: element.querySelector<ScoreItemInputTable>("d-score-item-input-table") + }; + + if (!isScoreItemElements(elements)) { + console.error("Missing required elements for score item editing"); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
app/assets/javascripts/score_item.ts(2 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (2)
app/assets/javascripts/score_item.ts (2)
3-3: LGTM: Import statement is correctly structured.
The import of ScoreItemInputTable aligns with the PR objective of integrating the spreadsheet-like table editor.
49-50: Verify the impact of removing initItemVisibilityCheckboxes.
The function has been simplified to only call initTotalVisibilityCheckboxes. Let's verify there are no remaining references to the removed functionality.
There was a problem hiding this comment.
- When hovering over the tables (which is editable on click), set the cursor to
textto hint at editability - The flow to add a score item is not very clear. It becomes clear when you are in edit mode, but it is not clear that you must enter edit mode to add an item. I would at least add a button/link to the empty state. In addition, instead of just a pencil icon, we could maybe add/replace it with an "edit scores" button
- Likewise, it is not clear/easy to remove a row. I think the only way is to right-click a row header?
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.
config/locales/js/en.yml
Outdated
| jspreadsheet: | ||
| copy: Copy... | ||
| deleteSelectedRows: Delete selected rows | ||
| insertNewRowAfter: Insert new row after | ||
| insertNewRowBefore: Insert new row before | ||
| paste: Paste... |
There was a problem hiding this comment.
Add missing translations for reordering functionality
The PR objectives mention support for reordering score items, but there are no translations for drag-and-drop or reordering UI elements.
Consider adding the following translations:
jspreadsheet:
copy: Copy...
deleteSelectedRows: Delete selected rows
insertNewRowAfter: Insert new row after
insertNewRowBefore: Insert new row before
paste: Paste...
+ drag_handle: Drag to reorder
+ drop_here: Drop here to move📝 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.
| jspreadsheet: | |
| copy: Copy... | |
| deleteSelectedRows: Delete selected rows | |
| insertNewRowAfter: Insert new row after | |
| insertNewRowBefore: Insert new row before | |
| paste: Paste... | |
| jspreadsheet: | |
| copy: Copy... | |
| deleteSelectedRows: Delete selected rows | |
| insertNewRowAfter: Insert new row after | |
| insertNewRowBefore: Insert new row before | |
| paste: Paste... | |
| drag_handle: Drag to reorder | |
| drop_here: Drop here to move |
config/locales/js/nl.yml
Outdated
| jspreadsheet: | ||
| copy: Kopiëer... | ||
| deleteSelectedRows: Verwijder geselecteerde rijen | ||
| insertNewRowAfter: Voeg nieuwe rij toe na deze | ||
| insertNewRowBefore: Voeg nieuwe rij toe voor deze | ||
| paste: Plak... |
There was a problem hiding this comment.
Fix spelling and consistency in spreadsheet actions.
There are spelling and consistency issues in the translations:
- copy: Kopiëer...
+ copy: Kopieer…
- paste: Plak...
+ paste: Plak…Note:
- Fixed Dutch spelling of "Kopieer"
- Replaced three dots with proper ellipsis character (…)
📝 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.
| jspreadsheet: | |
| copy: Kopiëer... | |
| deleteSelectedRows: Verwijder geselecteerde rijen | |
| insertNewRowAfter: Voeg nieuwe rij toe na deze | |
| insertNewRowBefore: Voeg nieuwe rij toe voor deze | |
| paste: Plak... | |
| jspreadsheet: | |
| copy: Kopieer… | |
| deleteSelectedRows: Verwijder geselecteerde rijen | |
| insertNewRowAfter: Voeg nieuwe rij toe na deze | |
| insertNewRowBefore: Voeg nieuwe rij toe voor deze | |
| paste: Plak… |
| "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.", | ||
| "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." | ||
| }, |
There was a problem hiding this comment.
Add warning message for reordering score items
The PR mentions that reordering score items after solutions have been evaluated may require reevaluation, but there's no warning message for this in the translations.
Add these translations:
"score_items": {
+ "reorder_warning": "Reordering score items after solutions have been evaluated will require reevaluation of those solutions.",
...
}And for Dutch:
"score_items": {
+ "reorder_warning": "Het herordenen van scoreonderdelen nadat oplossingen zijn geëvalueerd, vereist een herevaluatie van die oplossingen.",
...
}Also applies to: 849-869
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.
test.mp4
Darkmode is also supported
Closes #4940 and closes #2713