Create dice roll widget

[briananders]

Add a new interactive post featuring a customizable dice rolling widget.

---

 

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[claude]

Pull Request Review: Dice Roller Widget

Overview

This PR adds a new interactive dice rolling widget for tabletop gaming. The implementation is well-structured and follows most of the project's conventions. Below is my detailed review:

✅ Strengths

Code Quality

• Clean structure: JavaScript follows the established pattern with ready.document() wrapper
• Good documentation: Functions have JSDoc comments explaining parameters and return values
• Proper event handling: Event listeners are properly attached and include keyboard support (Enter key)
• Animation implementation: The rolling animation with temporary values is a nice UX touch

Accessibility

• The number input includes proper min/max attributes
• Keyboard support allows Enter key to trigger rolls

⚠️ Issues & Recommendations

1. CSS Variables Don't Exist in Project (High Priority)

Location: src/styles/posts/dice-roller.scss:7,46,173,185

The SCSS uses CSS variables that don't exist in this codebase:

• --background-secondary
• --background-primary
• --background-tertiary
• --text-primary
• --accent-color
• --border-color

Fix: Use the project's SCSS variables instead:

// Replace these:
background: var(--background-secondary);
color: var(--text-primary);
border: 2px solid var(--border-color);
background: var(--accent-color);

// With these:
background: $palette--hover-grey;
color: $palette--primary-grey;
border: 2px solid $palette--divider-color;
background: $palette--accent-color;

2. Inconsistent Class Naming (Medium Priority)

Location: src/templates/posts/dice-roller.ejs:14

The template is missing the required pageClasses wrapper class in the template structure. Compare with color-canvas.ejs:

Current (incorrect):

pageClasses:
  - 'posts dice-roller'

Should be (based on SCSS):

pageClasses:
  - 'posts'
  - 'dice-roller'

The SCSS selector .posts.dice-roller expects both classes on the body/main element.

3. Missing Spacing Units (Medium Priority)

Location: Throughout dice-roller.scss

The project uses a standardized spacing system ($s1, $s2, $s3, etc. based on 6px units), but this file uses magic numbers:

// Current:
margin: 2rem 0;
padding: 2rem;
gap: 1.5rem;
padding: 0.75rem;

// Should use:
margin: $s4 0;      // 24px
padding: $s4;       // 24px  
gap: $s3;           // 18px
padding: $s2;       // 12px

This ensures consistency with the rest of the site's spacing.

4. Date Inconsistency (Low Priority)

Location: src/templates/posts/dice-roller.ejs:4

date: 2024-12-10

This is dated in 2024, but today is December 10, 2025. Should this be 2025-12-10?

5. Input Validation Could Be Improved (Low Priority)

Location: src/js/posts/dice-roller.js:104-108

The validation shows an alert() which is acceptable but not ideal. Consider:

• Using inline error messages styled with the site's design
• The alert doesn't prevent invalid states (users can still blur with invalid values)
• Consider using <input min="1" max="20"> HTML5 validation attributes (already set but not leveraged)

6. Magic Numbers (Low Priority)

Location: src/js/posts/dice-roller.js:38-40

const ANIMATION_DURATION = 500;
const ANIMATION_INTERVAL = 50;

These are well-named constants (good!), but they're defined inside the function. Consider moving them to the top level for easier tuning:

const ANIMATION_DURATION = 500;
const ANIMATION_INTERVAL = 50;

ready.document(() => {
  // ... rest of code

7. Potential Animation Memory Leak (Low Priority)

Location: src/js/posts/dice-roller.js:51-53

While unlikely to be an issue in practice, the animate function could theoretically be called again before the previous animation completes if the button is re-enabled. Consider storing the timeout ID and clearing it if a new roll starts.

🔒 Security

✅ No security concerns identified:

• No external data sources
• No DOM innerHTML manipulation
• No eval() or unsafe practices
• Input validation present

🚀 Performance

✅ Performance looks good:

• Animation uses setTimeout which is appropriate for this use case
• DOM manipulation is minimal during animation
• No memory leaks identified (minor improvement suggested above)

🧪 Test Coverage

⚠️ No tests included. While this is a simple interactive widget, consider adding tests for:

• rollDie() returns values in correct range
• rollDice() returns correct array length
• Edge cases: rolling 1 die, rolling 20 dice, different die sides

📝 Suggested Action Items

Must Fix Before Merge:

1. Replace all CSS variables with project SCSS variables
2. Fix the pageClasses format in the EJS template

Should Fix Before Merge:
3. Update spacing to use project's spacing units ($s1, $s2, etc.)
4. Verify the date is correct (2024 vs 2025)

Nice to Have:
5. Move animation constants to top level
6. Improve input validation UX
7. Add basic unit tests

Summary

This is a solid implementation that will work well once the CSS variables are corrected. The JavaScript code is clean and well-documented, and the animation adds a nice touch. The main issue is using CSS variables that don't exist in this project—switching to the project's SCSS variables will fix the styling issues.

Great work overall! 🎲

[briananders]

Pull Request Review: Dice Roller Widget

Overview

This PR adds a new interactive dice rolling widget for tabletop gaming. The implementation is well-structured and follows most of the project's conventions. Below is my detailed review:

✅ Strengths

Code Quality

• Clean structure: JavaScript follows the established pattern with ready.document() wrapper
• Good documentation: Functions have JSDoc comments explaining parameters and return values
• Proper event handling: Event listeners are properly attached and include keyboard support (Enter key)
• Animation implementation: The rolling animation with temporary values is a nice UX touch

Accessibility

• The number input includes proper min/max attributes
• Keyboard support allows Enter key to trigger rolls

⚠️ Issues & Recommendations

1. CSS Variables Don't Exist in Project (High Priority)

Location: src/styles/posts/dice-roller.scss:7,46,173,185

The SCSS uses CSS variables that don't exist in this codebase:

• --background-secondary
• --background-primary
• --background-tertiary
• --text-primary
• --accent-color
• --border-color

Fix: Use the project's SCSS variables instead:

// Replace these:
background: var(--background-secondary);
color: var(--text-primary);
border: 2px solid var(--border-color);
background: var(--accent-color);

// With these:
background: $palette--hover-grey;
color: $palette--primary-grey;
border: 2px solid $palette--divider-color;
background: $palette--accent-color;

2. Inconsistent Class Naming (Medium Priority)

Location: src/templates/posts/dice-roller.ejs:14

The template is missing the required pageClasses wrapper class in the template structure. Compare with color-canvas.ejs:

Current (incorrect):

pageClasses:
  - 'posts dice-roller'

Should be (based on SCSS):

pageClasses:
  - 'posts'
  - 'dice-roller'

The SCSS selector .posts.dice-roller expects both classes on the body/main element.

3. Missing Spacing Units (Medium Priority)

Location: Throughout dice-roller.scss

The project uses a standardized spacing system ($s1, $s2, $s3, etc. based on 6px units), but this file uses magic numbers:

// Current:
margin: 2rem 0;
padding: 2rem;
gap: 1.5rem;
padding: 0.75rem;

// Should use:
margin: $s4 0;      // 24px
padding: $s4;       // 24px  
gap: $s3;           // 18px
padding: $s2;       // 12px

This ensures consistency with the rest of the site's spacing.

4. Date Inconsistency (Low Priority)

Location: src/templates/posts/dice-roller.ejs:4

date: 2024-12-10

This is dated in 2024, but today is December 10, 2025. Should this be 2025-12-10?

5. Input Validation Could Be Improved (Low Priority)

Location: src/js/posts/dice-roller.js:104-108

The validation shows an alert() which is acceptable but not ideal. Consider:

• Using inline error messages styled with the site's design
• The alert doesn't prevent invalid states (users can still blur with invalid values)
• Consider using <input min="1" max="20"> HTML5 validation attributes (already set but not leveraged)

6. Magic Numbers (Low Priority)

Location: src/js/posts/dice-roller.js:38-40

const ANIMATION_DURATION = 500;
const ANIMATION_INTERVAL = 50;

These are well-named constants (good!), but they're defined inside the function. Consider moving them to the top level for easier tuning:

const ANIMATION_DURATION = 500;
const ANIMATION_INTERVAL = 50;

ready.document(() => {
  // ... rest of code

7. Potential Animation Memory Leak (Low Priority)

Location: src/js/posts/dice-roller.js:51-53

While unlikely to be an issue in practice, the animate function could theoretically be called again before the previous animation completes if the button is re-enabled. Consider storing the timeout ID and clearing it if a new roll starts.

🔒 Security

✅ No security concerns identified:

• No external data sources
• No DOM innerHTML manipulation
• No eval() or unsafe practices
• Input validation present

🚀 Performance

✅ Performance looks good:

• Animation uses setTimeout which is appropriate for this use case
• DOM manipulation is minimal during animation
• No memory leaks identified (minor improvement suggested above)

🧪 Test Coverage

⚠️ No tests included. While this is a simple interactive widget, consider adding tests for:

• rollDie() returns values in correct range
• rollDice() returns correct array length
• Edge cases: rolling 1 die, rolling 20 dice, different die sides

📝 Suggested Action Items

Must Fix Before Merge:

1. Replace all CSS variables with project SCSS variables
2. Fix the pageClasses format in the EJS template

Should Fix Before Merge: 3. Update spacing to use project's spacing units ($s1, $s2, etc.) 4. Verify the date is correct (2024 vs 2025)

Nice to Have: 5. Move animation constants to top level 6. Improve input validation UX 7. Add basic unit tests

Summary

This is a solid implementation that will work well once the CSS variables are corrected. The JavaScript code is clean and well-documented, and the animation adds a nice touch. The main issue is using CSS variables that don't exist in this project—switching to the project's SCSS variables will fix the styling issues.

Great work overall! 🎲

@cursoragent update the pull requests to resolve the concerns raised within this feedback