Skip to content

Conversation

@tvolk131
Copy link
Contributor

The logic of h_button() and h_small_button() are almost identical. The only differences are a few size and spacing parameters, and h_small_button()'s support for showing only the svg icon if the button text is empty, which shouldn't impact current usage of h_button(). This PR pulls the shared logic into a helper function, reducing the implementation of these two existing functions to a single line call.

Depends on #258

Copilot AI review requested due to automatic review settings August 20, 2025 04:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the button components by extracting shared logic from h_button() and h_small_button() into a common helper function h_button_inner(). The refactoring reduces code duplication by parametrizing the differences between the two button types (size, spacing, border width, and height) while maintaining their existing behavior.

  • Extracted common button implementation into h_button_inner() with configurable parameters
  • Updated both h_button() and h_small_button() to use the shared implementation
  • Fixed text color logic to consistently handle disabled state in both button types

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
harbor-ui/src/components/button.rs Refactored button components to share common implementation logic
harbor-ui/src/routes/receive.rs Updated button usage to conditionally add event handler based on input state

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

};

let text_color = if loading { gray } else { Color::WHITE };
let text_color = if loading || matches!(status, Status::Disabled) {
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text color logic now includes disabled state handling that wasn't present in the original h_small_button() implementation. This change in behavior should be verified to ensure it doesn't break existing UI expectations for small buttons in disabled state.

Suggested change
let text_color = if loading || matches!(status, Status::Disabled) {
let text_color = if loading {

Copilot uses AI. Check for mistakes.
@tvolk131 tvolk131 closed this Sep 2, 2025
@tvolk131 tvolk131 deleted the cleanup_buttons branch September 2, 2025 16:13
@tvolk131 tvolk131 restored the cleanup_buttons branch September 3, 2025 16:05
@tvolk131 tvolk131 reopened this Sep 3, 2025
@tvolk131
Copy link
Contributor Author

tvolk131 commented Sep 3, 2025

Whoops, not sure how/why I closed this lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant