feat: highlight button for active dpns subscreen #51
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/ui/components/dpns_subscreen_chooser_panel.rs (1)
42-53: LGTM with minor suggestions for improvementThe button styling and click handling implementation is clean and effective. Consider these optional improvements:
- Move color constants to a theme configuration for better maintainability
- Consider using a const map for subscreen to RootScreenType mapping to reduce the match statement complexity
Example theme configuration approach:
const BUTTON_COLORS: ButtonTheme = ButtonTheme { active: (Color32::from_rgb(0, 128, 255), Color32::WHITE), inactive: (Color32::GRAY, Color32::WHITE), };src/ui/dpns_contested_names_screen.rs (4)
Line range hint
823-890: Consider extracting error message display logicThe error message display logic in the UI method could be extracted into a separate method for better maintainability and reusability. This would help reduce the complexity of the
uimethod and make the code more modular.Consider refactoring like this:
impl DPNSContestedNamesScreen { + fn render_error_message(&mut self, ui: &mut Ui) { + if let Some((message, message_type, _)) = self.error_message.clone() { + if message_type != MessageType::Success { + let message_color = match message_type { + MessageType::Error => egui::Color32::RED, + MessageType::Info => egui::Color32::BLACK, + MessageType::Success => unreachable!(), + }; + + ui.add_space(10.0); + ui.allocate_ui(egui::Vec2::new(ui.available_width(), 50.0), |ui| { + ui.group(|ui| { + ui.set_min_height(50.0); + ui.horizontal_wrapped(|ui| { + ui.label(egui::RichText::new(message).color(message_color)); + if ui.button("Dismiss").clicked() { + self.dismiss_error(); + } + }); + }); + }); + ui.add_space(10.0); + } + } + } fn ui(&mut self, ctx: &Context) -> AppAction { // ... existing code ... - let error_message = self.error_message.clone(); - if let Some((message, message_type, _)) = error_message { - // ... error message display code ... - } + self.render_error_message(ui); // ... rest of the method ... } }
Line range hint
432-790: Consider extracting common table setup logicThe table rendering methods (
render_table_active_contests,render_table_past_contests, andrender_table_local_dpns_names) share similar setup patterns. Consider extracting the common table setup logic into a reusable helper method to reduce code duplication.Here's a suggested approach:
impl DPNSContestedNamesScreen { fn setup_table_frame<'a>(&self, ui: &'a mut Ui) -> egui::Frame { Frame::group(ui.style()) .fill(ui.visuals().panel_fill) .stroke(egui::Stroke::new( 1.0, ui.visuals().widgets.inactive.bg_stroke.color, )) .inner_margin(Margin::same(8.0)) } fn setup_table_builder(ui: &mut Ui) -> TableBuilder { TableBuilder::new(ui) .striped(true) .resizable(true) .cell_layout(egui::Layout::left_to_right(egui::Align::Center)) } }This would allow you to simplify the table rendering methods and maintain consistency across all table implementations.
Line range hint
27-39: Consider enhancing sort implementation with traitsThe current sorting implementation could be improved by using Rust's trait system to make it more type-safe and reusable.
Consider implementing a trait-based approach:
trait Sortable { fn sort_by(&mut self, column: SortColumn, order: SortOrder); } impl Sortable for Vec<ContestedName> { fn sort_by(&mut self, column: SortColumn, order: SortOrder) { self.sort_by(|a, b| { let cmp = match column { SortColumn::ContestedName => a.normalized_contested_name.cmp(&b.normalized_contested_name), SortColumn::LockedVotes => a.locked_votes.cmp(&b.locked_votes), // ... other cases ... }; if order == SortOrder::Descending { cmp.reverse() } else { cmp } }); } }This would make the sorting logic more maintainable and reusable across different table implementations.
Also applies to: 147-170
Line range hint
1-890: Consider implementing the Model-View-Presenter patternThe current implementation mixes UI rendering, state management, and business logic. Consider adopting the MVP pattern to:
- Separate concerns between data management and UI rendering
- Improve testability
- Make the code more maintainable
This would involve:
- Creating a separate Presenter struct for business logic
- Moving state management to a Model struct
- Keeping only UI-specific code in the Screen struct
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/ui/components/dpns_subscreen_chooser_panel.rs(3 hunks)src/ui/dpns_contested_names_screen.rs(1 hunks)
🔇 Additional comments (2)
src/ui/components/dpns_subscreen_chooser_panel.rs (1)
1-7: LGTM: Clean imports and well-structured function signature
The new imports and updated function signature properly support the feature's requirements for accessing application context.
src/ui/dpns_contested_names_screen.rs (1)
823-823: LGTM: Function call updated correctly
The modification to add_dpns_subscreen_chooser_panel correctly passes the application context reference, maintaining proper ownership semantics with as_ref().
ogabrielides
force-pushed
the
fix/dpns-screens
branch
from
9468b8d to
5f5311a
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ogabrielides
left a comment
ogabrielides
left a comment
There was a problem hiding this comment.
LGTM, rebased and applied coderabbit suggestion
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/ui/components/dpns_subscreen_chooser_panel.rs (1)
40-49: Consider accessibility improvements for button colors.While the button styling implementation is clean, consider:
- Extracting color constants to improve maintainability
- Ensuring sufficient color contrast for accessibility
Consider this refactor:
+ const ACTIVE_BUTTON_COLOR: Color32 = Color32::from_rgb(0, 128, 255); + const INACTIVE_BUTTON_COLOR: Color32 = Color32::GRAY; + const BUTTON_TEXT_COLOR: Color32 = Color32::WHITE; let (button_color, text_color) = if is_active { - (Color32::from_rgb(0, 128, 255), Color32::WHITE) + (ACTIVE_BUTTON_COLOR, BUTTON_TEXT_COLOR) } else { - (Color32::GRAY, Color32::WHITE) + (INACTIVE_BUTTON_COLOR, BUTTON_TEXT_COLOR) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/ui/components/dpns_subscreen_chooser_panel.rs(3 hunks)src/ui/dpns_contested_names_screen.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/dpns_contested_names_screen.rs
🔇 Additional comments (3)
src/ui/components/dpns_subscreen_chooser_panel.rs (3)
1-7: LGTM! Clean imports and well-structured function signature.
The new imports and updated function signature with app_context parameter are properly organized and necessary for the implementation.
16-24: Great improvement in error handling!
The new implementation properly handles potential errors in settings retrieval with:
- Proper pattern matching for both Result and Option
- Safe fallback to Active screen when settings are unavailable
- Clear and maintainable code structure
This successfully addresses the concerns raised in the previous review.
Line range hint 51-71: LGTM! Clean and well-structured navigation logic.
The button click handling implementation is:
- Well-organized with clear pattern matching
- Properly maps subscreens to corresponding root screen types
- Follows Rust idioms for state transitions
Summary by CodeRabbit
New Features
Bug Fixes