-
Notifications
You must be signed in to change notification settings - Fork 1
fix: constants refactor #25
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| # Allonsh Constants Refactor Summary | ||
|
|
||
| ## Overview | ||
|
|
||
| This document summarizes the refactoring work done to replace hardcoded values with meaningful constants throughout the Allonsh library. | ||
|
|
||
| ## Files Created/Modified | ||
|
|
||
| ### 1. New Constants File: `src/constants.js` | ||
|
|
||
| - **Z_INDEX**: Centralized z-index values (GHOST: 999, DRAGGING: 1000, DROPPED: 9999, etc.) | ||
| - **DEFAULTS**: Default configuration values (stack spacing, direction) | ||
| - **CSS_CLASSES**: Consistent CSS class names | ||
| - **CSS_POSITIONS**: Position values (relative, absolute) | ||
| - **CSS_CURSORS**: Cursor values (grab, grabbing) | ||
| - **OPACITY**: Opacity values (ghost: 0.3, full: 1) | ||
| - **EVENTS**: Custom event names | ||
| - **FLEX_DIRECTIONS**: Flexbox direction values | ||
| - **DISPLAY_MODES**: Display property values | ||
| - **POINTER_EVENTS**: Pointer-events values | ||
|
|
||
| ### 2. Main Library: `src/allonsh.js` | ||
|
|
||
| **Replaced hardcoded values with constants:** | ||
|
|
||
| #### Z-Index Values: | ||
|
|
||
| - `999` → `Z_INDEX.GHOST` | ||
| - `1000` → `Z_INDEX.DRAGGING` | ||
| - `9999` → `Z_INDEX.DROPPED` | ||
|
|
||
| #### Default Values: | ||
|
|
||
| - `'horizontal'` → `DEFAULTS.STACK_DIRECTION` | ||
| - `5` → `DEFAULTS.STACK_SPACING` | ||
|
|
||
| #### CSS Class Names: | ||
|
|
||
| - `'restricted'` → `CSS_CLASSES.RESTRICTED` | ||
|
|
||
| #### CSS Properties: | ||
|
|
||
| - `'relative'` → `CSS_POSITIONS.RELATIVE` | ||
| - `'absolute'` → `CSS_POSITIONS.ABSOLUTE` | ||
| - `'grab'` → `CSS_CURSORS.GRAB` | ||
| - `'grabbing'` → `CSS_CURSORS.GRABBING` | ||
| - `'flex'` → `DISPLAY_MODES.FLEX` | ||
| - `'row'` → `FLEX_DIRECTIONS.ROW` | ||
| - `'column'` → `FLEX_DIRECTIONS.COLUMN` | ||
| - `'wrap'` → `FLEX_WRAP` | ||
|
|
||
| #### Opacity Values: | ||
|
|
||
| - `'0.3'` → `OPACITY.GHOST` | ||
| - `'1'` → `OPACITY.FULL` | ||
|
|
||
| #### Pointer Events: | ||
|
|
||
| - `'none'` → `POINTER_EVENTS.NONE` | ||
| - `'auto'` → `POINTER_EVENTS.AUTO` | ||
|
|
||
| #### Event Names: | ||
|
|
||
| ### 3. Demo Script: `demo/js/script.js` | ||
|
|
||
| - Imported constants for default values | ||
| - Replaced hardcoded `10` with `DEFAULTS.STACK_SPACING_DEMO` | ||
|
|
||
| ### 4. Test File: `test/allonsh.test.js` | ||
|
|
||
| - Imported constants for event names | ||
| - Replaced hardcoded event name with `EVENTS.DROP` | ||
|
|
||
| ### 5. CSS File: `demo/css/styles.css` | ||
|
|
||
| - Added comments indicating z-index values could be updated to use CSS custom properties | ||
| - Values remain hardcoded for now but are documented for future refactoring | ||
|
|
||
| ## Benefits Achieved | ||
|
|
||
| 1. **Maintainability**: All hardcoded values are now centralized in one location | ||
| 2. **Consistency**: Values are guaranteed to be consistent across the codebase | ||
| 3. **Self-documenting**: Constants have meaningful names that explain their purpose | ||
| 4. **Easy Updates**: Changing a value requires updating only the constants file | ||
| 5. **Type Safety**: Better IDE support and error detection | ||
| 6. **Code Clarity**: Intent is clearer with named constants instead of magic numbers | ||
|
|
||
| ## Future Improvements | ||
|
|
||
| 1. **CSS Custom Properties**: Z-index values in CSS could be moved to CSS custom properties | ||
| 2. **Theme System**: Constants could be extended to support theme-based values | ||
| 3. **Configuration**: Constants could be made configurable at runtime | ||
| 4. **Validation**: Add validation to ensure constant values are within expected ranges | ||
|
|
||
| ## Impact | ||
|
|
||
| - **High Priority**: This was a quick win with massive maintainability improvement | ||
| - **Low Risk**: Changes are purely internal refactoring with no API changes | ||
| - **High Value**: Makes the codebase significantly easier to maintain and extend |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ import Allonsh from '../../src/allonsh.js'; | |||||||||||||||
| const openPanelBtn = document.getElementById('openPanelBtn'); | ||||||||||||||||
| const closePanelBtn = document.getElementById('closePanelBtn'); | ||||||||||||||||
|
|
||||||||||||||||
| import { DEFAULTS, CSS_CLASSES, EVENTS } from '../../src/constants.js'; | ||||||||||||||||
| const controlPanel = document.getElementById('controlPanel'); | ||||||||||||||||
|
|
||||||||||||||||
| const toggleStackCheckbox = document.getElementById('stackCheckbox'); | ||||||||||||||||
|
|
@@ -38,7 +39,9 @@ let enableStackingBool = true; | |||||||||||||||
| let restrictDropzoneBool = true; | ||||||||||||||||
|
|
||||||||||||||||
| let ghostEffectBool = true; | ||||||||||||||||
| let stackSpacingDefault = 10; | ||||||||||||||||
| import { DEFAULTS } from '../../src/constants.js'; | ||||||||||||||||
|
|
||||||||||||||||
| let stackSpacingDefault = DEFAULTS.STACK_SPACING_DEMO; | ||||||||||||||||
|
|
||||||||||||||||
| function initAllonsh(options) { | ||||||||||||||||
| if (allonshInstance) { | ||||||||||||||||
|
|
@@ -95,8 +98,8 @@ function updateStackDirectionButtons() { | |||||||||||||||
| verticalStackBtn.classList.add('selected'); | ||||||||||||||||
| horizontalStackBtn.classList.remove('selected'); | ||||||||||||||||
| } else { | ||||||||||||||||
| verticalStackBtn.classList.remove('selected'); | ||||||||||||||||
| horizontalStackBtn.classList.add('selected'); | ||||||||||||||||
| const draggableSelectorClass = CSS_CLASSES.DRAGGABLE; | ||||||||||||||||
| const dropzoneSelectorClass = CSS_CLASSES.DROPZONE; | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+101
to
103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restore button state toggling for horizontal case. The else-branch no longer toggles Apply this diff: } else {
- const draggableSelectorClass = CSS_CLASSES.DRAGGABLE;
- const dropzoneSelectorClass = CSS_CLASSES.DROPZONE;
+ horizontalStackBtn.classList.add('selected');
+ verticalStackBtn.classList.remove('selected');
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @p55d2k, Please revert these changes so that the class toggle continues to function correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -142,7 +145,7 @@ resetBtn.addEventListener('click', () => { | |||||||||||||||
| enableStackingBool = true; | ||||||||||||||||
| restrictDropzoneBool = true; | ||||||||||||||||
| ghostEffectBool = true; | ||||||||||||||||
| stackSpacingDefault = 10; | ||||||||||||||||
| stackSpacingDefault = DEFAULTS.STACK_SPACING_DEMO; | ||||||||||||||||
| currentStackDirection = 'horizontal'; | ||||||||||||||||
| setDefaultsOnLoad(); | ||||||||||||||||
| updateStackDirectionButtons(); | ||||||||||||||||
|
|
@@ -177,18 +180,20 @@ restrictDropzoneCheckbox.addEventListener('change', () => { | |||||||||||||||
| function logDraggablesOutsideDropzones() { | ||||||||||||||||
| const playAreaElement = document.querySelector(`.${playAreaSelectorClass}`); | ||||||||||||||||
|
|
||||||||||||||||
| playAreaElement.querySelectorAll('.draggable').forEach((element) => { | ||||||||||||||||
| if ( | ||||||||||||||||
| ![...playAreaElement.querySelectorAll('.dropzone')].some((dropzone) => | ||||||||||||||||
| dropzone.contains(element) | ||||||||||||||||
| ) | ||||||||||||||||
| ) { | ||||||||||||||||
| element.style.position = 'relative'; | ||||||||||||||||
| element.style.left = ''; | ||||||||||||||||
| element.style.top = ''; | ||||||||||||||||
| draggableContainer.appendChild(element); | ||||||||||||||||
| } | ||||||||||||||||
| }); | ||||||||||||||||
| playAreaElement | ||||||||||||||||
| .querySelectorAll(`.${CSS_CLASSES.DRAGGABLE}`) | ||||||||||||||||
| .forEach((element) => { | ||||||||||||||||
| if ( | ||||||||||||||||
| ![...playAreaElement.querySelectorAll(`.${CSS_CLASSES.DROPZONE}`)].some( | ||||||||||||||||
| (dropzone) => dropzone.contains(element) | ||||||||||||||||
| ) | ||||||||||||||||
| ) { | ||||||||||||||||
| element.style.position = 'relative'; | ||||||||||||||||
| element.style.left = ''; | ||||||||||||||||
| element.style.top = ''; | ||||||||||||||||
| draggableContainer.appendChild(element); | ||||||||||||||||
| } | ||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| openPanelBtn.addEventListener('click', () => { | ||||||||||||||||
|
|
@@ -204,7 +209,7 @@ function togglePanel() { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| function emptyDropzones() { | ||||||||||||||||
| const dropzones = document.querySelectorAll('.dropzone'); | ||||||||||||||||
| const dropzones = document.querySelectorAll(`.${CSS_CLASSES.DROPZONE}`); | ||||||||||||||||
| dropzones.forEach((dropzone) => { | ||||||||||||||||
| while (dropzone.firstChild) { | ||||||||||||||||
| dropzone.removeChild(dropzone.firstChild); | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -82,15 +82,24 @@ Allonsh dispatches the following custom events on relevant elements: | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| ```js | ||||||||||||||||||||||||||||||||||||||
| import Allonsh from 'allonsh'; | ||||||||||||||||||||||||||||||||||||||
| import { CSS_CLASSES, EVENTS } from 'allonsh/constants'; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const dragDrop = new Allonsh({ | ||||||||||||||||||||||||||||||||||||||
| draggableSelector: 'draggable', | ||||||||||||||||||||||||||||||||||||||
| dropzoneSelector: 'dropzone', | ||||||||||||||||||||||||||||||||||||||
| draggableSelector: CSS_CLASSES.DRAGGABLE, | ||||||||||||||||||||||||||||||||||||||
| dropzoneSelector: CSS_CLASSES.DROPZONE, | ||||||||||||||||||||||||||||||||||||||
| playAreaSelector: 'play-area', | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+85
to
90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify that The docs import from a subpath. Ensure your package.json “exports” includes Run: 🏁 Script executed: #!/bin/bash
set -euo pipefail
jq '.exports' package.json || { echo "No exports field"; exit 1; }
echo "Looking for ./constants subpath…"
jq -r '.exports|tostring' package.json | rg -n "\./constants" -n || echo "Missing ./constants export"Length of output: 256 🏁 Script executed: #!/bin/bash
# Find all files named constants.* in the repository
fd -t f 'constants.*'Length of output: 70 Add subpath export for The import • File to update: package.json Suggested diff (adjust paths to match your bundle outputs): {
"name": "allonsh",
"version": "1.0.0",
- "main": "src/index.js",
+ "main": "src/index.js",
+ "exports": {
+ ".": "./src/index.js",
+ "./constants": "./src/constants.js"
+ },
// …other fields…
}After this change, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
| restrictToDropzones: true, | ||||||||||||||||||||||||||||||||||||||
| enableStacking: true, | ||||||||||||||||||||||||||||||||||||||
| stackDirection: 'vertical', | ||||||||||||||||||||||||||||||||||||||
| stackSpacing: 10, | ||||||||||||||||||||||||||||||||||||||
| useGhostEffect: true, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Listen for custom events | ||||||||||||||||||||||||||||||||||||||
| document.addEventListener(EVENTS.DRAG_START, (e) => { | ||||||||||||||||||||||||||||||||||||||
| // handle drag start | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| document.addEventListener(EVENTS.DROP, (e) => { | ||||||||||||||||||||||||||||||||||||||
| // handle drop | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix duplicate import and remove unused identifier.
DEFAULTSis imported twice (Lines 6 and 42), triggering a linter error. Also,EVENTSis imported but never used.Apply this diff:
Also applies to: 42-42
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @p55d2k, please address this issue as it is breaking the demo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.