Conversation
Pull Request ReviewThis PR updates the Prima hooks to use LiveView's ✅ Positive Observations1. Consistent API Usage
2. Improved ID Management
3. Code Quality
|
PR Review: Revert require idsSummaryThis PR reverts the requirement for explicit IDs on function components and migrates all direct DOM attribute manipulations to use ✅ Strengths1. Consistent API Usage 2. Automatic ID Generation 3. Comprehensive Coverage 🔍 Code Quality Observations1. Redundant setAttribute Calls in dropdown.js In // Lines 336-337 - old code not using this.js()
button.setAttribute('aria-controls', menu.id)
menu.setAttribute('aria-labelledby', button.id)
// Lines 339-342 - new code with this.js() doing the same thing
if (!button.id) this.js().setAttribute(button, 'id', triggerId)
this.js().setAttribute(button, 'aria-controls', menuId)
if (!menu.id) this.js().setAttribute(menu, 'id', menuId)
this.js().setAttribute(menu, 'aria-labelledby', triggerId)Recommendation: Remove lines 336-337 as they:
2. Potential Issue with Section Heading ID Generation In this.js().setAttribute(firstChild, 'id', `${dropdownId}-section-${sectionIndex}-heading`)However, Recommendation: Either:
3. Missing Error Handling The code assumes setupAriaRelationships(button, menu) {
const dropdownId = this.el.id
if (!dropdownId) {
console.error('Dropdown requires an id attribute')
return
}
// ... rest of code
}🧪 Testing ConsiderationsTest Coverage: The PR doesn't include new tests. Given that:
Recommendations:
🔒 Security & PerformanceNo security concerns identified. The changes are limited to attribute management and don't introduce XSS or injection risks. Performance impact is negligible - the
|
Code Review for PR #23: Revert require idsSummaryThis PR migrates all direct DOM attribute manipulation to use LiveView's Critical Bug Found 🐛Location: setupSectionLabels() {
const sections = this.el.querySelectorAll('[role="group"]')
sections.forEach((section) => {
const firstChild = section.firstElementChild
if (firstChild && firstChild.getAttribute('role') === 'presentation') {
if (!firstChild.id) {
this.js().setAttribute(firstChild, 'id', `${dropdownId}-section-${sectionIndex}-heading`)
// ^^^^^^^^^^^ undefined variable
// ^^^^^^^^^^^^ undefined variable
}
this.js().setAttribute(section, 'aria-labelledby', firstChild.id)
}
})
}Issue: Fix needed: setupSectionLabels() {
const dropdownId = this.el.id // Add this
const sections = this.el.querySelectorAll('[role="group"]')
sections.forEach((section, sectionIndex) => { // Add sectionIndex parameter
const firstChild = section.firstElementChild
if (firstChild && firstChild.getAttribute('role') === 'presentation') {
if (!firstChild.id) {
this.js().setAttribute(firstChild, 'id', `${dropdownId}-section-${sectionIndex}-heading`)
}
this.js().setAttribute(section, 'aria-labelledby', firstChild.id)
}
})
}Code Quality & Best Practices ✅Strengths:
Improvements made:
Potential Issues & Concerns1. Dependency on upstream PR
2. ID uniqueness assumptions
3. Missing tests for new ID generation logic
Performance Considerations ✅No performance concerns identified. The changes are straightforward replacements that should have similar or better performance characteristics since LiveView can batch DOM updates more efficiently. Security Concerns ✅No security issues found. The changes are internal refactoring and don't introduce user-controlled input or XSS vulnerabilities. Test Coverage
|
PR Review: Revert require IDsSummaryThis PR successfully refactors all three component hooks (dropdown, combobox, modal) to use Positive ChangesArchitecture Improvements
Code Quality
Issues FoundCritical Bug in Dropdown ComponentLocation: this.js().setAttribute(firstChild, 'id', `${dropdownId}-section-${sectionIndex}-heading`)❌ Bug: Fix needed: sections.forEach((section, sectionIndex) => { // Add sectionIndex parameter
const firstChild = section.firstElementChild
if (firstChild && firstChild.getAttribute('role') === 'presentation') {
if (!firstChild.id) {
this.js().setAttribute(firstChild, 'id', `${dropdownId}-section-${sectionIndex}-heading`)
}
this.js().setAttribute(section, 'aria-labelledby', firstChild.id)
}
})This bug would cause runtime errors when dropdowns have sections/groups. RecommendationsTesting
Code ConsistencyThe refactoring is largely consistent, but consider extracting the ID generation pattern into a helper since it's repeated across all three components: ensureElementId(element, fallbackId) {
if (!element.id) {
this.js().setAttribute(element, 'id', fallbackId)
}
return element.id
}Security & Performance
Blocking Issues
Overall AssessmentStrong architectural improvement that aligns with LiveView best practices. The code is cleaner and more maintainable. However, the Once the bug is fixed and tests pass, this will be a solid improvement to the library. 👍 |
PR Review: Revert require idsSummaryThis PR migrates all DOM attribute manipulation to use Phoenix LiveView's this.js().setAttribute() API instead of direct DOM manipulation. This change depends on phoenix_live_view PR 4146 which provides these new APIs. Code Quality - PASSStrengths:
Refactoring improvements in dropdown.js:
Potential Issues - NEEDS ATTENTION1. CRITICAL BUG in dropdown.js:370The code references an undefined variable sectionIndex: this.js().setAttribute(firstChild, 'id', dropdownId + '-section-' + sectionIndex + '-heading') Issue: Variable sectionIndex is undefined. The forEach callback doesn't provide an index parameter. Fix needed: 2. Inconsistent event listener binding in dropdown.js:63-67The PR changes direct method references to bound methods but removes the handle* wrapper pattern used elsewhere in the file. Other handlers like handleMouseOver, handleMenuClick, handleKeydown follow the handle* naming convention. Recommendation: Either keep the wrapper methods for consistency with the codebase pattern, OR document why these specific events differ from the pattern. 3. Incomplete migration in dropdown.js:293-294The PR migrates setAttribute() and removeAttribute() calls but still uses direct style manipulation (style.display). While this may be intentional, it's worth considering consistency. Question: Should style.display changes also use a LiveView API, or is direct manipulation appropriate here? Performance Considerations - PASS
Security Concerns - PASSNo security issues identified:
Test Coverage - NEEDS VERIFICATIONRecommendations:
Additional Notes
VerdictNEEDS FIXES BEFORE MERGE The PR is well-structured and represents a good improvement, but requires fixing the sectionIndex bug in dropdown.js:370 before merging. The other concerns are minor and can be addressed based on maintainer preference. Action Items:
|
Pull Request ReviewOverviewThis PR makes a significant architectural improvement by removing the requirement for explicit IDs on dropdown elements and using ✅ Strengths1. Improved Developer Experience
2. Consistent API Pattern
3. Enhanced Test Coverage
4. Clean Refactoring
🔍 Code Quality Observations1. Accessibility Preserved
2. JavaScript Hook Patterns // Before: Multiple redundant wrapper functions
handleClose() { this.hideMenu() }
handleToggle() { this.toggleMenu() }
showMenuAndFocusFirst() { /* duplicated show logic */ }
// After: Direct binding and extracted showMenu()
[this.refs.button, 'click', this.toggleMenu.bind(this)]
[this.el, 'prima:close', this.hideMenu.bind(this)]
showMenu() { /* reusable logic */ }3. ID Generation Logic
|
PR Review: Revert require idsSummaryThis PR removes the requirement for explicit IDs on dropdown components and migrates all DOM attribute manipulation to use Code Quality & Best Practices ✅Strengths:
Observations:
Accessibility ✅Excellent ARIA improvements:
The test coverage validates:
Potential Issues & Concerns1. Missing null/undefined checks
|
1 participant
this.js().setAttribute()for all attributesDepends on phoenixframework/phoenix_live_view#4146