fix: improve Sort By dropdown styling consistency #2363

mrkeshav-05 · 2025-10-05T21:54:48Z

Remove blue outline and inconsistent hover backgrounds
Add proper focus/active states styles to match the design system
Improve light/dark mode theme integration
Fix trigger background blending issues

Before Changes:

Light Mode	Dark Mode

After Changes:

Light Mode	Dark Mode

coderabbitai · 2025-10-05T21:55:13Z

Summary by CodeRabbit

Style
- Enhanced Sort By visuals with borders, shadows, transitions, and consistent dark mode styling.
- Refined Select dropdown (label, trigger, value, popover, list) with clearer hover, focus, and selected states.
- Improved Select item responsiveness and state feedback.
- Updated sort order button with borders, hover/focus rings, and dark-mode variants.
- Harmonized arrow icon colors for dark mode.
- No functional changes; interactions remain the same.

Walkthrough

Restyles the Sort By UI in frontend/src/components/SortBy.tsx: adds borders, shadows, transitions, hover/focus rings, expanded Select/SelectItem/listbox classes, dark-mode color adjustments, and updates FontAwesome icon colors. No data-flow or event-handler changes.

Changes

Cohort / File(s)	Summary
Sort By UI restyle `frontend/src/components/SortBy.tsx`	Adds border, border-color, shadow, hover/transition and focus-ring styles to the outer container and sort-order button; updates Select/label/trigger/value/selectorIcon/popoverContent/listbox classNames to include hover/focus/selected and dark-mode states; expands `SelectItem` styling for hover/focus/selected/responsive states; adjusts FontAwesome icon colors. No logic or API changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Fixed SortBy component test cases #2173 #2180 — Modifies SortBy UI structure/behavior; relevant for comparing dropdown/button presentation and hidden combobox changes.
Feature/test sortby component 1915 #1950 — Adds unit tests for frontend/src/components/SortBy.tsx; relevant for validating rendering and interactions after styling changes.

Suggested reviewers

arkid15r
kasya

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title “fix: improve Sort By dropdown styling consistency” directly reflects the primary change of the pull request by summarizing the styling improvements for the Sort By dropdown in a concise and clear manner.
Linked Issues Check	✅ Passed	The pull request implements all coding objectives from issue #1372 by removing the default blue outline, applying custom focus and active state styles consistent with the design system, redesigning the dropdown container with appropriate padding, borders, and backgrounds, and ensuring both light and dark mode support as outlined in the issue.
Out of Scope Changes Check	✅ Passed	All modifications are confined to the Sort By dropdown styling in the specified component and directly map to the objectives from the linked issue, with no unrelated files or features altered.
Description Check	✅ Passed	The description outlines the key styling changes such as removing the blue outline, adding focus and active states, improving theme integration, and fixing background blending, and it references the related issue and before/after screenshots, making it clearly relevant to the changeset.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

frontend/src/components/SortBy.tsx (1)

27-28: Simplify redundant background declarations.

The trigger classes contain multiple redundant bg-transparent declarations across different states (hover, focus, data attributes), culminating in an !important flag. This complexity suggests difficulties overriding default component styles.

Consider investigating which specific state is causing the background issue and address it more surgically. If the library applies backgrounds at high specificity, document this in a code comment rather than using multiple overrides plus !important.

Additionally, verify that removing focus:outline-none and focus:ring-0 doesn't harm keyboard navigation accessibility, especially since no visible focus indicator appears to be provided for the trigger.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b2c811 and b6bf4b4.

📒 Files selected for processing (1)

frontend/src/components/SortBy.tsx (3 hunks)

🔇 Additional comments (3)

frontend/src/components/SortBy.tsx (3)

18-18: LGTM! Container styling is well-structured.

The outer container styling provides good visual hierarchy with appropriate borders, shadows, and smooth transitions. The light/dark mode color choices are consistent and enhance the overall user experience.

70-70: LGTM! Button styling includes proper accessibility features.

The sort order button has well-defined hover, focus, and active states with appropriate focus rings for keyboard navigation. The styling is consistent with the design system and provides good visual feedback.

75-80: LGTM! Icon colors provide good contrast.

The icon color updates maintain consistency between both sort direction icons and provide appropriate contrast in both light and dark modes.

frontend/src/components/SortBy.tsx

kasya

Thanks @mrkeshav-05 for working on this!
Left some comments.

Also, please run make check and commit suggested changes - this is required by contributing guidelines in OWASP Nest project.

kasya · 2025-10-05T23:23:31Z

frontend/src/components/SortBy.tsx

Can we make sure the padding on right and left side match? Right now it's way bigger on the left:

Fixed the padding issue - added balanced px-3 to the container

frontend/src/components/SortBy.tsx

mrkeshav-05 · 2025-10-06T10:24:11Z

@kasya Thank you for the review!

I've addressed the feedback:

Ran make check - all checks passed successfully
Fixed the padding imbalance issue - added balanced px-3 to the container
Added "hover:bg-transparent dark:hover:bg-transparent" for Light and Dark mode

screenshot of the improved style:

Light Mode	Dark Mode

Code is ready for review.

kasya · 2025-10-07T03:38:46Z

frontend/src/components/SortBy.tsx

    <div className="flex items-center gap-2">
      {/* Sort Attribute Dropdown */}
-      <div className="inline-flex h-12 items-center rounded-lg bg-gray-200 dark:bg-[#323232]">
+      <div className="inline-flex h-12 items-center rounded-lg border border-gray-300 bg-gray-100 px-3 shadow-sm transition-all duration-200 hover:shadow-md dark:border-gray-600 dark:bg-[#323232]">


I know the classes seem right, but we should have the same padding that we see on the frontend and not just the classes.
I don't think these are the same padding on right and left side 🤔 Clearly there's much more space after the caret on the right.

Which means that these classes should be different:

Suggested change

<div className="inline-flex h-12 items-center rounded-lg border border-gray-300 bg-gray-100 px-3 shadow-sm transition-all duration-200 hover:shadow-md dark:border-gray-600 dark:bg-[#323232]">

<div className="inline-flex h-12 items-center rounded-lg border border-gray-300 bg-gray-100 pl-3 shadow-sm transition-all duration-200 hover:shadow-md dark:border-gray-600 dark:bg-[#323232]">

To only keep the padding (we don't need padding after caret to match the left side).
Before and after:

Yes, you're right! Previously with px-3, the right side had too much padding because it added:

px-3 (padding on both sides) = 12px right padding

Plus the natural spacing from the dropdown icon/caret

These are the after changes:

In this padding of 12px are on the both side.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 308b164 and 3e399f6.

⛔ Files ignored due to path filters (3)

backend/poetry.lock is excluded by !**/*.lock
docs/poetry.lock is excluded by !**/*.lock
frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (5)

README.md (1 hunks)
backend/pyproject.toml (1 hunks)
frontend/docker/Dockerfile.e2e.test (1 hunks)
frontend/package.json (4 hunks)
frontend/src/components/SortBy.tsx (3 hunks)

✅ Files skipped from review due to trivial changes (2)

frontend/docker/Dockerfile.e2e.test
README.md

🔇 Additional comments (1)

backend/pyproject.toml (1)

45-47: Slack SDK bump is compatible with slack-bolt 1.22.0

slack-bolt 1.22.0 requires slack_sdk>=3.33.5,<4, so ^3.37.0 is valid; no changes needed.

Likely an incorrect or invalid review comment.

frontend/src/components/SortBy.tsx

- Remove blue outline and inconsistent hover backgrounds - Add proper focus/active states styles to match the design system - Improve light/dark mode theme integration - Fix trigger background blending issues Fixes OWASP#1372

sonarqubecloud · 2025-10-07T21:07:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

frontend/src/components/SortBy.tsx (1)
26-27: Restore accessible focus indicator on the trigger.

The trigger removes the default focus outline (focus:outline-none focus:ring-0) without providing a replacement, which breaks keyboard accessibility. This issue was previously flagged but remains unresolved.

Add a custom focus-visible indicator such as data-[focus-visible=true]:ring-2 data-[focus-visible=true]:ring-gray-300 dark:data-[focus-visible=true]:ring-gray-500 to ensure keyboard users can see which element has focus.

Apply this diff to add accessible focus styling:
             trigger:
-              'bg-transparent data-[hover=true]:bg-transparent focus:outline-none focus:ring-0 border-none shadow-none text-nowrap w-32 min-h-8 h-8 text-sm font-medium text-gray-800 dark:text-gray-200 hover:text-gray-900 dark:hover:text-gray-100 transition-all duration-0',
+              'bg-transparent data-[hover=true]:bg-transparent focus:outline-none focus:ring-0 data-[focus-visible=true]:ring-2 data-[focus-visible=true]:ring-gray-300 dark:data-[focus-visible=true]:ring-gray-500 data-[focus-visible=true]:ring-offset-2 border-none shadow-none text-nowrap w-32 min-h-8 h-8 text-sm font-medium text-gray-800 dark:text-gray-200 hover:text-gray-900 dark:hover:text-gray-100 transition-all duration-0',

🧹 Nitpick comments (1)

frontend/src/components/SortBy.tsx (1)

27-27: Remove redundant transition-all when using duration-0. The transition-all utility combined with duration-0 effectively disables transitions—drop transition-all if no transitions are desired.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bbd5f1 and bdf0957.

📒 Files selected for processing (1)

frontend/src/components/SortBy.tsx (3 hunks)

🔇 Additional comments (4)

frontend/src/components/SortBy.tsx (4)

18-18: LGTM!

The container styling is well-structured with appropriate borders, shadows, hover effects, and comprehensive dark mode support. The use of pl-3 (left padding only) correctly addresses the padding balance issue discussed in previous reviews.

44-46: Excellent! Hover behavior is now consistent.

The SelectItem styling now uses hover:bg-transparent dark:hover:bg-transparent, ensuring consistent hover behavior across light and dark themes. This successfully addresses the previous review feedback.

The comprehensive state handling for focus, selection, and hover states with appropriate transitions demonstrates good attention to detail.

65-65: LGTM!

The sort order button styling is excellent with proper focus ring (focus:ring-2 focus:ring-gray-300 dark:focus:ring-gray-500) that maintains accessibility while removing the default outline. The comprehensive hover, transition, and dark mode support is well-implemented.

68-76: LGTM!

The FontAwesome icon color updates (text-gray-600 dark:text-gray-300) provide excellent consistency across light and dark themes. The styling is appropriately applied to both ascending and descending order icons.

kasya

LGTM! Thanks!

mrkeshav-05 requested review from arkid15r and kasya as code owners October 5, 2025 21:54

github-actions bot added the frontend label Oct 5, 2025

coderabbitai bot reviewed Oct 5, 2025

View reviewed changes

frontend/src/components/SortBy.tsx Outdated Show resolved Hide resolved

kasya requested changes Oct 5, 2025

View reviewed changes

kasya requested changes Oct 7, 2025

View reviewed changes

github-actions bot added docs Improvements or additions to documentation backend docker Pull requests that update Docker code labels Oct 7, 2025

coderabbitai bot reviewed Oct 7, 2025

View reviewed changes

frontend/src/components/SortBy.tsx Show resolved Hide resolved

mrkeshav-05 force-pushed the fix/sort-by-dropdown-styling-1372 branch from 3e399f6 to 4bbd5f1 Compare October 7, 2025 20:19

github-actions bot removed docs Improvements or additions to documentation backend docker Pull requests that update Docker code labels Oct 7, 2025

fix: improve Sort By dropdown styling consistency

bdf0957

- Remove blue outline and inconsistent hover backgrounds - Add proper focus/active states styles to match the design system - Improve light/dark mode theme integration - Fix trigger background blending issues Fixes OWASP#1372

mrkeshav-05 force-pushed the fix/sort-by-dropdown-styling-1372 branch from 4bbd5f1 to bdf0957 Compare October 7, 2025 21:06

coderabbitai bot reviewed Oct 7, 2025

View reviewed changes

kasya approved these changes Oct 8, 2025

View reviewed changes

kasya added this pull request to the merge queue Oct 8, 2025

Merged via the queue into OWASP:main with commit 68eb222 Oct 8, 2025
25 checks passed

mrkeshav-05 deleted the fix/sort-by-dropdown-styling-1372 branch October 8, 2025 06:43

arkid15r added hacktoberfest hacktoberfest-accepted labels Oct 13, 2025

This was referenced Nov 27, 2025

fix: add keyboard navigation support #2737

Open

fix: add missing type="button" attributes to button elements #2747

Closed

	<div className="inline-flex h-12 items-center rounded-lg border border-gray-300 bg-gray-100 px-3 shadow-sm transition-all duration-200 hover:shadow-md dark:border-gray-600 dark:bg-[#323232]">
	<div className="inline-flex h-12 items-center rounded-lg border border-gray-300 bg-gray-100 pl-3 shadow-sm transition-all duration-200 hover:shadow-md dark:border-gray-600 dark:bg-[#323232]">

Uh oh!

fix: improve Sort By dropdown styling consistency #2363

fix: improve Sort By dropdown styling consistency #2363

Uh oh!

Conversation

mrkeshav-05 commented Oct 5, 2025

Uh oh!

coderabbitai bot commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kasya left a comment

Choose a reason for hiding this comment

Uh oh!

kasya Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

mrkeshav-05 Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrkeshav-05 commented Oct 6, 2025

Uh oh!

kasya Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

mrkeshav-05 Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 7, 2025

Quality Gate passed

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

kasya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Oct 5, 2025 •

edited

Loading