rusackas commented on PR #37141:
URL: https://github.com/apache/superset/pull/37141#issuecomment-3923425089
## Review Feedback Response
Thank you for the thorough review! I've addressed several issues and will
explain the approach for each:
### Fixed in latest commit:
1. **MobileRouteGuard sessionStorage** (index.tsx:80) - ✅ Fixed. Added
try/catch wrapper and initialized state safely.
2. **MobileUnsupported sessionStorage** (index.tsx:59) - ✅ Fixed. Wrapped
setItem in try/catch so "Continue anyway" still works without crashing.
3. **MobileUnsupported misleading comment** (index.tsx:64) - ✅ Fixed.
Updated comment to accurately describe that it shows a hint rather than
redirecting.
4. **SubMenu test data-test vs data-testid** (SubMenu.test.tsx:157) - ✅
Fixed. Changed to data-testid for findByTestId compatibility.
5. **Home useBreakpoint initial render** (index.tsx:150) - ✅ Fixed. Added
default `= true` to prevent flash on initial render.
6. **DashboardList mobile search aria-label** (index.tsx:752) - ✅ Fixed.
Added `aria-label={t('Search')}` for accessibility.
### Acknowledged but deferring:
7. **service-worker.js dev build artifacts** - This file should not have
been modified. It appears to be from a local build artifact. I'll investigate
and ensure it's reverted or properly built for production.
8. **ListView filter ref in drawer** (ListView.tsx:600) - Valid point. The
ref is needed for clearFilters. Will address in follow-up.
9. **ListView viewMode check for sort** (ListView.tsx:601) - Agreed, should
check viewMode. Will address in follow-up.
10. **ListView desktop-sort CSS** (ListView.tsx:453) - Good catch about
conditional className. Will address in follow-up.
11. **DashboardBuilder hasFilters guard** (DashboardBuilder.tsx:522) - Will
investigate the edge case with chart customizations.
12. **ListView duplicate FilterControls** (ListView.tsx:642) - Valid
optimization suggestion. Will consider destroyOnClose approach.
13. **ListView viewMode state reactivity** (utils.ts:259) - Worth
investigating further.
14. **i18n concatenated strings** (useHeaderActionsDropdownMenu.tsx:237) -
Valid i18n improvement. Will refactor to use interpolation.
Most critical issues are fixed. The ListView and i18n improvements will be
addressed in follow-up commits to keep the PR focused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
