Conversation
WalkthroughReworks global theming into a structured token system, namespaces and migrates landing-page styles under .landing-page, updates main layout search/MultiSelect styling and pills, initializes MultiSelect with a "searching" class, and adds a Dash callback to toggle the "searching" class during search interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Search UI (MultiSelect)
participant DashCB as Dash Callback
participant CSS as Stylesheet Engine
User->>UI: Click search icon (n_clicks++)
UI->>DashCB: emit search.n_clicks
activate DashCB
DashCB->>DashCB: inspect dash.ctx.triggered / triggered_id
DashCB->>UI: add "searching" to projects.className
deactivate DashCB
UI->>CSS: render with "searching"
CSS->>UI: apply active pill styles (blue/white)
User->>UI: change projects selection (value)
UI->>DashCB: emit projects.value
activate DashCB
DashCB->>DashCB: inspect dash.ctx.triggered / triggered_id
DashCB->>UI: remove "searching" from projects.className
deactivate DashCB
UI->>CSS: render without "searching"
CSS->>UI: apply default pill styles (gray/white)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
EngCaioFonseca
force-pushed
the
UI-fixez
branch
from
f6eb718 to
e94df41
Compare
EngCaioFonseca
moved this from Backlog
to Testing | Waiting on PR response | Blocked
in Aspen Project Board
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
8Knot/pages/index/index_layout.py (1)
216-250: Reconsider the default className to align with the expected initial state.The MultiSelect is initialized with
className="searchbar-dropdown searching", but according to the PR objectives and help text, pills should be grey initially and only turn blue when the search icon is clicked. However, the CSS rules in8Knot/assets/main_layout.css(lines 406-410) apply a blue background when thesearchingclass is present:.searchbar-dropdown.searching .mantine-MultiSelect-pill, .searchbar-dropdown.searching .mantine-MultiSelect-value { background-color: var(--baby-blue-500) !important; color: var(--color-white) !important; }This creates a conflict where the initial state has the
searchingclass (indicating active search) but the inline styles (lines 242-249) set a grey background. This relies on CSS specificity to override the!importantblue background, which is fragile and confusing.Recommendation: Initialize the className as
"searchbar-dropdown"(withoutsearching) so pills start grey, then let the callback inindex_callbacks.pyadd thesearchingclass when the user clicks the search button.Apply this diff:
- className="searchbar-dropdown searching", + className="searchbar-dropdown",
🧹 Nitpick comments (1)
8Knot/pages/index/index_layout.py (1)
242-249: Consider removing inline pill/value styles to avoid duplication.The inline styles for
"value"and"pill"(grey background) duplicate the CSS rules already defined in8Knot/assets/main_layout.css(lines 394-398). This creates maintenance overhead and reduces flexibility for theming.Recommendation: Remove these inline style definitions and rely solely on the CSS in
main_layout.cssfor consistency. The CSS already provides the default grey styling for.searchbar-dropdown .mantine-MultiSelect-pilland.searchbar-dropdown .mantine-MultiSelect-value.Apply this diff:
styles={ "input": { "fontSize": "16px", "minHeight": "48px", "height": "auto", "padding": "12px 16px 12px 44px", "borderRadius": "20px", "display": "flex", "flexWrap": "wrap", "alignItems": "flex-start", "backgroundColor": "#1D1D1D", "borderColor": "#404040", "position": "relative", "zIndex": 1, }, "dropdown": { "borderRadius": "12px", "backgroundColor": "#1D1D1D", "border": "1px solid #444", }, "item": { "borderRadius": "8px", "margin": "2px 4px", "color": "white", }, - "value": { - "backgroundColor": "#555555", - "color": "white", - }, - "pill": { - "backgroundColor": "#555555", - "color": "white", - }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
8Knot/assets/landing_page.css(5 hunks)8Knot/assets/main_layout.css(4 hunks)8Knot/pages/index/index_callbacks.py(1 hunks)8Knot/pages/index/index_layout.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
8Knot/pages/index/index_callbacks.py
872-872: Unused function argument: selected_values
(ARG001)
🔇 Additional comments (8)
8Knot/pages/index/index_layout.py (1)
280-281: LGTM!The extended help text clearly communicates the search confirmation behavior to users, explaining that the search is only active when they click the search icon and the pill turns blue. This improves the user experience by reducing confusion about the search state.
8Knot/assets/main_layout.css (2)
92-125: LGTM!The styling additions improve sidebar clarity:
- The distinction between
span.sidebar-section-text(text next to icons) anda.sidebar-section-text(standalone link text) is well-documented with comments.- The nested item indentation (
.sidebar-dropdown-container .collapse a) enhances visual hierarchy.
276-289: LGTM!The searchbar input background overrides are necessary to handle Dash Bootstrap Components' default styles. The comments clearly explain the specificity requirements, and the selectors are appropriately scoped to avoid unintended side effects.
8Knot/assets/landing_page.css (5)
244-248: LGTM!The tab hover styling changes align with the PR objectives to change the hover color to baby blue. Removing
!importantdeclarations improves CSS maintainability and specificity management. The new hover color (--baby-blue-300) provides clearer visual feedback.
304-308: LGTM!The updates to
.section-borderedimprove consistency by using CSS variables (--color-topbar-bgand--border-radius-md) for background and border-radius. This makes theming more maintainable.
345-347: LGTM!The
.feature-imagestyling updates use CSS variables (--border-radius-mdand--color-border) for consistency across the application. The simplified border declaration is cleaner.
1054-1068: LGTM!The arrow styling updates align with the PR objective to make graph arrows white. The
filter: brightness(0) invert(1)approach is a clean CSS-only solution for color conversion. Removing!importantdeclarations improves maintainability.
1100-1103: LGTM!The responsive arrow styling correctly applies rotation for vertical mobile layout while maintaining the white color filter. The approach is consistent with the desktop styling.
EngCaioFonseca
force-pushed
the
UI-fixez
branch
from
e94df41 to
e96220e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
8Knot/assets/main_layout.css (1)
281-306: Consolidate redundant placeholder styling rules.Lines 281-286, 291-296, and 301-306 all target the same placeholder pseudo-element with identical property values. These rules can be merged into a single selector to reduce CSS verbosity:
-/* Override placeholder text background color */ - -.searchbar-dropdown input::placeholder { - background-color: var(--placeholder-bg-color); - color: var(--placeholder-text-color); - font-size: var(--placeholder-font-size); - line-height: var(--placeholder-line-height); -} - - -/* Ensure the placeholder background matches when focused */ - -.searchbar-dropdown input:focus::placeholder { - background-color: var(--placeholder-bg-color); - color: var(--placeholder-text-color); - font-size: var(--placeholder-font-size); - line-height: var(--placeholder-line-height); -} - - -/* Override any Mantine-specific placeholder styling */ - -.mantine-MultiSelect-input::placeholder { +/* Override all placeholder styling variants */ + +.searchbar-dropdown input::placeholder, +.searchbar-dropdown input:focus::placeholder, +.mantine-MultiSelect-input::placeholder { background-color: var(--placeholder-bg-color); color: var(--placeholder-text-color); font-size: var(--placeholder-font-size); line-height: var(--placeholder-line-height); }8Knot/assets/landing_page.css (1)
14-30: Landing-specific token definitions are well-organized and maintainable.The new spacing and typography tokens (lines 14-30) are clearly scoped to landing-specific usage with consistent naming conventions (
--landing-spacing-*,--landing-font-*,--landing-radius-*). This reduces global namespace pollution and makes tokens easy to locate and update.Consider adding a comment block above the token definitions explaining their scope and intended usage, to help future maintainers understand why these are separate from global tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
8Knot/assets/landing_page.css(16 hunks)8Knot/assets/main_layout.css(9 hunks)8Knot/pages/index/index_callbacks.py(1 hunks)8Knot/pages/index/index_layout.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- 8Knot/pages/index/index_layout.py
- 8Knot/pages/index/index_callbacks.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: End-to-end test (Docker)
- GitHub Check: End-to-end test (Podman)
🔇 Additional comments (5)
8Knot/assets/landing_page.css (4)
813-820: Arrow styling effectively implements white arrow design per PR objectives.The CSS filter
brightness(0) invert(1)on line 819 successfully transforms the arrow image to solid white as specified in the PR objectives ("Graph arrows in the first tab are now white"). This approach is cross-browser compatible and doesn't require SVG modifications.
851-856: Mobile arrow styling consistently implements white arrow design.The mobile responsive rule at line 855 applies the same CSS filter approach (
brightness(0) invert(1)) to maintain white arrow styling on smaller screens. Therotate(90deg)transform correctly reorients the arrow for the vertical layout.
174-201: Tab styling transitions cleanly to token-based system with proper specificity.The
.landing-page .tabs-headerrules now use comprehensive token references (--color-topbar-bg,--baby-blue-500,--border-radius-lg) with increased CSS specificity through the.landing-pagenamespace. The explicit.landing-pagescoping prevents unintended global styling bleed.
245-251: Tab hover color successfully implements PR objective with properly defined tokens.The hover state for tabs now uses
var(--baby-blue-300)(#76C5EF) instead of grey, correctly implementing the PR objective to change tabs' hover color for better UX. Verification confirms both--baby-blue-300and--baby-blue-500are properly defined incolor.css.8Knot/assets/main_layout.css (1)
381-420: Remove duplicate pill styling—CSS variables are properly defined.The duplication concern is valid (lines 394–398 are redundant since
.searchbar-dropdownis always the parent), but the original review incorrectly flagged--gray-mediumas undefined. Verification confirms both variables exist incolor.css:
--gray-medium: #555555(line 46)--baby-blue-500: #199AD6(line 37)Remove the generic rule at lines 394–398 to consolidate:
/* Style for filter pills (grey by default when selected) */ -.mantine-MultiSelect-pill, -.mantine-MultiSelect-value { - background-color: var(--gray-medium); - color: var(--color-white); -} - .searchbar-dropdown .mantine-MultiSelect-pill, .searchbar-dropdown .mantine-MultiSelect-value { background-color: var(--gray-medium); color: var(--color-white); }The existing CSS variables require no substitution; they are correctly defined and used.
Likely an incorrect or invalid review comment.
EngCaioFonseca
force-pushed
the
UI-fixez
branch
from
53e2874 to
53a9405
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
8Knot/assets/main_layout.css (1)
394-404: Consolidate duplicate pill styling and replace undefined CSS variable.This section has two issues flagged in previous reviews that remain unresolved:
Duplication: Lines 394-398 and 400-404 define identical grey background styling. Since MultiSelect is always within
.searchbar-dropdown, the more specific second rule (lines 400-404) is sufficient.Undefined CSS variable:
--gray-mediumis used but not defined in8Knot/assets/color.css. You should replace it with an available token like--content-subtle(#C7C7C7).Apply this diff:
/* Style for filter pills (grey by default when selected) */ -.mantine-MultiSelect-pill, -.mantine-MultiSelect-value { - background-color: var(--gray-medium); - color: var(--color-white); -} - .searchbar-dropdown .mantine-MultiSelect-pill, .searchbar-dropdown .mantine-MultiSelect-value { - background-color: var(--gray-medium); + background-color: var(--content-subtle); color: var(--color-white); }8Knot/pages/index/index_callbacks.py (1)
875-880: Remove unusedsearch_clicksparameter and add missing State parameter.The callback has two issues with its signature:
- The
search_clicksparameter (line 880) is declared but never used in the function body. You only checktriggered_id, not the click count.- The callback decorator shows
State("projects", "className")but the function signature lacks the correspondingcurrent_classnameparameter.Based on static analysis hints.
Apply this diff:
@callback( Output("projects", "className"), [Input("search", "n_clicks"), Input("projects", "value")], - prevent_initial_call=True, + [State("projects", "className")], + prevent_initial_call=True, ) -def update_pill_color_on_search(search_clicks, selected_values): +def update_pill_color_on_search(search_clicks, selected_values, current_classname): """Update pill color based on search action. When search icon is clicked, add 'searching' class to turn pills blue. When values change (user is selecting), remove 'searching' class to keep pills grey. """Note: If you don't need
search_clicksorcurrent_classnamein your logic, you can still keep them in the signature to match the callback declaration (Dash requires parameter order to match Inputs then States).
🧹 Nitpick comments (1)
8Knot/pages/index/index_callbacks.py (1)
893-898: Remove unnecessary f-string prefixes.Lines 893 and 897 use f-strings, but line 893 has no placeholders. Remove the
fprefix from line 893 for cleaner code.Based on static analysis hints.
Apply this diff:
if triggered_id == "search": # Search button clicked - add 'searching' class to turn pills blue - logging.info(f"PILL COLOR: Search clicked - turning pills BLUE") + logging.info("PILL COLOR: Search clicked - turning pills BLUE") return "searchbar-dropdown searching" elif triggered_id == "projects": # Values changed (user selecting) - remove 'searching' class to keep pills grey logging.info(f"PILL COLOR: Values changed - turning pills GREY. Selected: {selected_values}") return "searchbar-dropdown"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
8Knot/assets/landing_page.css(16 hunks)8Knot/assets/main_layout.css(9 hunks)8Knot/pages/index/index_callbacks.py(1 hunks)8Knot/pages/index/index_layout.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 8Knot/pages/index/index_layout.py
🧰 Additional context used
🪛 Ruff (0.14.3)
8Knot/pages/index/index_callbacks.py
880-880: Unused function argument: search_clicks
(ARG001)
893-893: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: End-to-end test (Docker)
- GitHub Check: End-to-end test (Podman)
🔇 Additional comments (4)
8Knot/assets/landing_page.css (3)
10-11: Documentation correctly references global color tokens.The comment accurately indicates this file now uses global CSS variables from
color.css, which aligns with the token-based refactoring throughout this PR.
810-817: Clean implementation of white arrow styling.Using
filter: brightness(0) invert(1)is an elegant CSS-only solution to make the arrow solid white without requiring image asset changes. This aligns with the PR objective of making graph arrows white in the first tab.
848-853: Consistent mobile arrow styling.The mobile responsive design correctly applies the same white filter styling and includes the 90-degree rotation for vertical layout. Good attention to detail maintaining consistency across breakpoints.
8Knot/assets/main_layout.css (1)
412-416: No issues found.The CSS variable
--baby-blue-500is defined in8Knot/assets/color.cssat line 37 with the value#199AD6. The code inmain_layout.csscorrectly references this defined variable.
EngCaioFonseca
force-pushed
the
UI-fixez
branch
from
5b18e96 to
4d7fc3e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
cdolfi
left a comment
cdolfi
left a comment
There was a problem hiding this comment.
Overall looks pretty good, seems like the rebase did not go through correctly with all the changes on assets.
Can the pill color be a less bright blue? I think itll be bit more visually appealing
8Knot/assets/color.css
Outdated
|
|
||
|
|
||
| /* ==== TYPOGRAPHY WITH HIGHER SPECIFICITY ==== */ | ||
|
|
There was a problem hiding this comment.
Seems like the rebase was not resolved correctlyu
EngCaioFonseca
force-pushed
the
UI-fixez
branch
from
f21593d to
3853ce6
Compare
cdolfi
left a comment
cdolfi
left a comment
There was a problem hiding this comment.
Put in some comments, in general code needs to be cleaned up and made non-generic
8Knot/assets/landing_page.css
Outdated
| } | ||
|
|
||
|
|
||
| /* font_transition: */ |
There was a problem hiding this comment.
Partially. The color of the tabs to baby-blue was going to be implemented in that PR (Font transitions), but then, I thought It'd be best to implement it here since this is about UI fixes anyways.
So, basically, the implementation about the colors changing the tabs are right, just the comment is not, which I'll change.
8Knot/assets/landing_page.css
Outdated
| } | ||
|
|
||
|
|
||
| /* UI-fixez: use background-color instead of color property */ |
There was a problem hiding this comment.
Update all "UI-fixez: " comments
There was a problem hiding this comment.
Shouldnt be in any of the in line comments
There was a problem hiding this comment.
no problem. I've only added these comments as temporary because before the rebase it was built on top of another branch, so it was showing a lot of modifications, not only the specifics for this one. I'll remove them now.
8Knot/assets/main_layout.css
Outdated
|
|
||
| /* Style for filter pills (grey by default when selected) */ | ||
|
|
||
| .mantine-MultiSelect-pill, |
There was a problem hiding this comment.
should be on color css, through 403
| # Works in conjunction with CSS in main_layout.css | ||
| # ============================================================================ | ||
| @callback( | ||
| Output("projects", "className"), |
There was a problem hiding this comment.
please make these variables descriptive to our codebase
8Knot/pages/index/index_layout.py
Outdated
| data=[augur.initial_multiselect_option()], | ||
| value=[augur.initial_multiselect_option()["value"]], | ||
| className="searchbar-dropdown", | ||
| # UI-fixez: Start with "searching" class so default selection (chaoss) shows as blue |
There was a problem hiding this comment.
Are there particular tools you are using to cause the PR title to make it into commits? usually I end up committing the code several times before even thinking of making a PR (and usually I let github title it for me based on the commit messages)
There was a problem hiding this comment.
No, this was a particular case, because before the rebase it was built on top of another PR, so I wanted to make sure you guys could actually see what were the modifications made for this specific PR. So, I've added the comments after the PR was already created.
There was a problem hiding this comment.
No worries on doing that in the future, you can select by commit(s) in review mode
… css to color.css.
…lared, but no usage of this value.
github-project-automation
bot
moved this from Testing | Waiting on PR response | Blocked
to Done
in Aspen Project Board
3 participants
This PR fixes a few things on the UI and aims to resolve confusion over what's been actively searching or not.
Fixes:
Summary by CodeRabbit
New Features
Style
Refactor