Drag and drop LED output reordering #5001
Conversation
WalkthroughAdds drag-and-drop reordering for LED output blocks, adds hDS/hDO/hDrop/recalcIds helpers, updates dynamic LED block HTML and naming (ids/names like l, LT/LL/LA/MA), and refactors per‑LED limiter handling by changing enLA(s) signature and switching current selectors to LL elements. Changes Cohort / File(s) Summary of Changes LED outputs drag-and-drop and limiter refactorwled00/data/settings_leds.htm Implemented drag-and-drop support by wiring ondragover/ondrop on the LED container and adding hDS, hDO, hDrop, and recalcIds; updated generated LED block HTML to be draggable (id="l<idx>", <span id="n...">, LL selects, etc.); changed enLA signature from enLA(s,n) to enLA(s) and adjusted code to derive bus/index from s.name; switched per-LED mA source references from LAsel<bus> to LL<bus> and updated related initialization and value-apply logic. Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches ✅ Passed checks (3 passed) Check name Status Explanation Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled. Title Check ✅ Passed The PR title "Drag and drop LED output reordering" directly and clearly summarizes the primary change in the pull request. The raw summary confirms that the changeset introduces drag-and-drop functionality for LED outputs with ondragover and ondrop handlers to enable reordering of LED output blocks. The PR objectives explicitly state the feature "adds the ability to reorder LED outputs via drag and drop." The title is concise, specific, and uses no vague or noise terminology. A teammate reviewing the commit history would immediately understand the primary functionality being added. Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped. ✨ Finishing touches 🧪 Generate unit tests (beta) [ ] Create PR with unit tests [ ] Post copyable unit tests in a comment [ ] Commit unit tests in branch dnd-output-reorder 📜 Recent review details Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 Commits Reviewing files that changed from the base of the PR and between 00fab2e and a18b133. 📒 Files selected for processing (1) wled00/data/settings_leds.htm (10 hunks) 🚧 Files skipped from review as they are similar to previous changes (1) wled00/data/settings_leds.htm ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19) GitHub Check: wled_build / Build Environments (usermods) GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi) GitHub Check: wled_build / Build Environments (esp8266_2m_compat) GitHub Check: wled_build / Build Environments (nodemcuv2_compat) GitHub Check: wled_build / Build Environments (esp01_1m_full_compat) GitHub Check: wled_build / Build Environments (esp32_eth) GitHub Check: wled_build / Build Environments (nodemcuv2) GitHub Check: wled_build / Build Environments (esp01_1m_full) GitHub Check: wled_build / Build Environments (esp8266_2m_160) GitHub Check: wled_build / Build Environments (esp01_1m_full_160) GitHub Check: wled_build / Build Environments (esp01_1m_full_160) GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi) GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi) GitHub Check: wled_build / Build Environments (usermods) GitHub Check: wled_build / Build Environments (esp32_wrover) GitHub Check: wled_build / Build Environments (esp32_eth) GitHub Check: wled_build / Build Environments (esp8266_2m_160) GitHub Check: wled_build / Build Environments (lolin_s2_mini) GitHub Check: Analyze (c-cpp) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ Share X Mastodon Reddit LinkedIn Comment @coderabbitai help to get the list of available commands and usage tips. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
134-136: Consider removing commented-out code.The commented-out lines appear to be dead code. If this logic is no longer needed, remove it to improve maintainability. If it's kept for reference, rely on version control history instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev)
🔇 Additional comments (3)
wled00/data/settings_leds.htm (3)
163-171: LGTM: Function signature refactored correctly.The signature change from
enLA(s,n)toenLA(s)is well-implemented. The bus number is now correctly derived from the select element's name usings.name.substring(2), making the function more self-contained and reducing the chance of passing incorrect parameters.
468-525: LGTM: Draggable HTML structure is well-implemented.The updated HTML structure correctly implements draggable LED blocks with:
- Proper drag attributes (
draggable="true",ondragstart)- Unique IDs for tracking (
id="l${s}")- Good UX feedback (
cursor:grab)- Consistent naming convention (
LL${s}instead ofLAsel${s})
638-657: LGTM: Comprehensive ID recalculation.The
recalcIds()function thoroughly handles all necessary updates after reordering:
- Display numbers (line 644)
- All name attributes for form elements (lines 646-649)
- All ID attributes for DOM elements (lines 651-655)
The implementation correctly maintains consistency across the entire LED output block structure.
| @@ -1,4 +1,4 @@ | |||
| <!DOCTYPE html> | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Remove the UTF-8 BOM character.
The file starts with a UTF-8 Byte Order Mark (). While technically valid, BOM characters can cause issues with some parsers, web servers, and build tools. It's generally recommended to save HTML files as UTF-8 without BOM.
🤖 Prompt for AI Agents
In wled00/data/settings_leds.htm around line 1, the file begins with a UTF-8 BOM
character which can break some parsers and tools; remove the BOM by deleting the
leading invisible character (or re-save the file as "UTF-8 without BOM" in your
editor/IDE) so the file starts with "<!DOCTYPE html>" and commit the change.
| if (n==1) { | ||
| // npm run build has trouble minimizing spaces inside string | ||
| var cn = `<div class="iST"> | ||
| var cn = `<div class="iST" draggable="true" ondragstart="hDS(event)" id="l${s}" style="cursor:grab;"> |
There was a problem hiding this comment.
Add keyboard navigation and ARIA attributes for accessibility.
The drag-and-drop implementation lacks keyboard navigation support and ARIA attributes, preventing users who rely on keyboards or screen readers from reordering LED outputs. This violates WCAG 2.1 accessibility guidelines.
Consider adding:
- Keyboard shortcuts (e.g., Ctrl+Up/Down arrows to reorder)
- Visible reorder buttons (↑/↓) as an alternative
- ARIA attributes:
role="list",role="listitem",aria-label="Drag to reorder",aria-grabbed,aria-dropeffect - Focus management after reordering
Example ARIA enhancement:
-<div class="iST" draggable="true" ondragstart="hDS(event)" id="l${s}" style="cursor:grab;">
+<div class="iST" draggable="true" ondragstart="hDS(event)" id="l${s}" style="cursor:grab;" role="listitem" aria-label="LED output ${i+1}, drag to reorder" tabindex="0">Also applies to: 621-637
🤖 Prompt for AI Agents
In wled00/data/settings_leds.htm around line 468 (and similarly 621-637), the
draggable LED item lacks keyboard navigation and ARIA attributes; add
accessibility by marking the container as role="list" and each item as
role="listitem" with aria-label="Drag to reorder", aria-grabbed (true/false) and
aria-dropeffect where appropriate, add tabindex="0" to make items focusable,
implement keyboard handlers (e.g., Ctrl+ArrowUp/Ctrl+ArrowDown or Home/End) to
move items programmatically and update aria-grabbed/dropeffect states, provide
visible ↑/↓ reorder buttons as alternatives that also update the underlying
order and focus, and ensure focus is moved to the item (or its button) after
reordering so screen reader users remain oriented.
| function hDS(e) { | ||
| e.dataTransfer.setData('text', e.currentTarget.id); | ||
| } | ||
| function hDO(e) { | ||
| e.preventDefault(); | ||
| } | ||
| function hDrop(e) { | ||
| e.preventDefault(); | ||
| let t = e.target; | ||
| if (t.id === "mLC") t = t.children[0]; // dropped on a container, not on an element | ||
| else while (t && !t.classList.contains("iST")) t = t.parentNode; // find target element | ||
| if (!t || t.id === "") return false; // not dropping on a valid target | ||
| const id = e.dataTransfer.getData("text"); | ||
| t.parentNode.insertBefore(gId(id), t); | ||
| recalcIds(); | ||
| UI(); | ||
| } |
There was a problem hiding this comment.
Minor: Handle edge case when dropping on empty container.
Line 630 assumes t.children[0] exists when dropping on the container. While unlikely in practice (there's always at least one LED output), this could cause a runtime error if the container is empty.
Consider adding a guard:
let t = e.target;
- if (t.id === "mLC") t = t.children[0]; // dropped on a container, not on an element
+ if (t.id === "mLC") {
+ if (!t.children[0]) return; // empty container
+ t = t.children[0];
+ }
else while (t && !t.classList.contains("iST")) t = t.parentNode; // find target element📝 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.
| function hDS(e) { | |
| e.dataTransfer.setData('text', e.currentTarget.id); | |
| } | |
| function hDO(e) { | |
| e.preventDefault(); | |
| } | |
| function hDrop(e) { | |
| e.preventDefault(); | |
| let t = e.target; | |
| if (t.id === "mLC") t = t.children[0]; // dropped on a container, not on an element | |
| else while (t && !t.classList.contains("iST")) t = t.parentNode; // find target element | |
| if (!t || t.id === "") return false; // not dropping on a valid target | |
| const id = e.dataTransfer.getData("text"); | |
| t.parentNode.insertBefore(gId(id), t); | |
| recalcIds(); | |
| UI(); | |
| } | |
| function hDrop(e) { | |
| e.preventDefault(); | |
| let t = e.target; | |
| if (t.id === "mLC") { | |
| if (!t.children[0]) return; // empty container | |
| t = t.children[0]; | |
| } | |
| else while (t && !t.classList.contains("iST")) t = t.parentNode; // find target element | |
| if (!t || t.id === "") return false; // not dropping on a valid target | |
| const id = e.dataTransfer.getData("text"); | |
| t.parentNode.insertBefore(gId(id), t); | |
| recalcIds(); | |
| UI(); | |
| } |
🤖 Prompt for AI Agents
In wled00/data/settings_leds.htm around lines 621 to 637, the drop handler
assumes t.children[0] exists when the drop target is the container ("mLC"); add
a guard so you don't access children[0] on an empty container: if the container
has no children, treat the container itself as the insertion point (or append
the dragged element to the container) instead of dereferencing children[0]; keep
the rest of the logic (recalcIds and UI) unchanged.
|
I tried this but what is the purpose?
can you elaborate? |
|
You can change only the last one, however now you can choose which is the last. |
|
On a C3 I can not change the last one: |
|
You found a bug. 🤦♂️ |
|
Do we need reordering as a feature or is this a workaround for the inability to edit all values of all outputs? |
You are welcome to find a solution how to dynamically enable certain LED types to prevent questions like "I've added 3 WS2812 outputs to my C3 but only 2 work. How come?" You can, however, just remove the line I changed in last commit if you don't want such protection. It is responsible for disabling select box. Disregarding above, there was a request long time ago to allow simpler reordering outputs. |
Ability to reorder LED outputs.
May allow users to update/modify otherwise unmodifiable output.
Summary by CodeRabbit
New Features
Refactor