kasiazjc commented on PR #40169: URL: https://github.com/apache/superset/pull/40169#issuecomment-4553999891
Thanks for the detailed second pass! **P0 datetime_range format** — fixed. Apply now calls `fetchTimeRange` directly, grabs the evaluated `since`/`until` from the API response, and submits `[since, until]` as an array. `convertFilters` hits the `Array.isArray` branch properly now. Also handled the `iso` vs `unix` distinction — unix timestamps get multiplied by 1000 before they land in filter state. **P1 fetchTimeRange spam** — fixed. The tooltip effect caches the last fetched value per index and skips if nothing changed, so unrelated filter edits don't trigger extra API calls. **P1 × in button** — yeah, structurally still inside the pill button. Fixing it properly means turning the pill into a flex container with two sibling buttons, which is more involved than I want to do in this PR. Filed it as a follow-up. The `aria-label` on the icon at least makes it readable by screen readers even if keyboard nav directly to the × is missing. **Cleanup** — done. `Select.tsx`, `Select.test.tsx`, `DateRange.tsx` all deleted. `dateFilterValueType` is now wired — `TimeRange.tsx` accepts it and handles unix → ms conversion before submitting. **Tests** — added `TimeRange.test.tsx` with 10 tests: Apply calls `onSubmit([since, until])`, `NO_TIME_RANGE` submits `undefined` without an extra API call, `clearFilter` via ref works, Apply is disabled until the API validates, etc. Also added two tests to `Filters/index.test.tsx` covering the onClear paths for datetime_range and numerical_range. **Re: sort pill** — changed this after your review, let me explain why. The PM flagged "sorting is broken" and the root cause was the pill showing plain "Sort" with no visual change at default, making it look like nothing happened when you picked Alphabetical (since Alphabetical is also the default, the pill just snapped back to the same label). The `isNonDefault + label-with-value` approach I had before fixed that visually but introduced a different inconsistency: the active value was in two places at once (label and tooltip) and the pill label was changing on every sort change. The bigger issue though is that sort is *always* active — there's no "no sort" state to clear back to, so a clear button makes no sense here. Putting the current sort value inline in the label also breaks the pill pattern: every other pill uses the label as the filter *name* and the tooltip as the active *value*. The sort pill should work the same way. Current behavior: always shows "Sort", current sort in tooltip on hover, no ×. I think that's actually correct — if you disagree happy to chat about it but I didn't want to leave the PM's bug unaddressed. -- 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]
