Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eagle 1372 #812

Merged
merged 16 commits into from
Jan 16, 2025
Merged

Eagle 1372 #812

merged 16 commits into from
Jan 16, 2025

Conversation

M-Wicenec
Copy link
Collaborator

@M-Wicenec M-Wicenec commented Jan 16, 2025

combining the close bottom window button instead of having one for each tab. this is just about reducing code.
ended up implementing a new hover effect for icons and buttons, because i noticed that most buttons didnt actually have one

Summary by Sourcery

Combine the close button for the bottom window into a single button instead of having one for each tab. Implement a new hover effect for icons and buttons.

New Features:

  • Add hover effect to icons and buttons.

Enhancements:

  • Reduce code complexity by combining close buttons.

Copy link
Contributor

sourcery-ai bot commented Jan 16, 2025

Reviewer's Guide by Sourcery

This PR combines the close buttons for the bottom window tabs into a single button. It also implements a new hover effect for icons and buttons.

State diagram for button hover states

stateDiagram-v2
    [*] --> Normal
    Normal --> Hover: Mouse over
    Hover --> Normal: Mouse out

    state Hover {
        [*] --> ApplyHoverColor
        ApplyHoverColor --> AddTextShadow
        AddTextShadow --> SetCursorPointer
    }
Loading

File-Level Changes

Change Details Files
Combined close button for bottom window tabs
  • Removed individual close buttons for each tab.
  • Added a single close button in the bottom window header.
templates/base.html
Implemented new hover effect for icons and buttons
  • Added a new CSS class iconHoverEffect for the hover effect.
  • Applied the iconHoverEffect class to various icons and buttons throughout the application.
  • Added a new color variable --hoverHighlight in static/base.css.
  • Set the hover highlight color to #feb609 in src/EagleConfig.ts.
  • Removed the old hover effect from static/base.css.
static/base.css
templates/inspector.html
templates/node_parameter_table.html
templates/navbar.html
static/tables.css
templates/base.html
templates/graph_configurations_table.html
static/components/repository.html
static/components/repository-file.html
templates/palettes.html
templates/Errors.html
templates/modals/settings.html
templates/config_parameter_table.html
static/components/palette-component.html
templates/modals/errors.html

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@M-Wicenec M-Wicenec merged commit b15da00 into master Jan 16, 2025
1 check passed
@M-Wicenec M-Wicenec deleted the eagle-1372 branch January 16, 2025 08:16
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @M-Wicenec - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider restructuring the CSS selectors to avoid using !important tags, as these can make styles harder to maintain and override. Try increasing selector specificity instead.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +2751 to +2756
.iconHoverEffect:hover {
color: var(--hoverHighlight) !important;
cursor: pointer;
border-color: var(--hoverHighlight) !important;
text-shadow: -1px -1px 0 #4b4b4b, 0 -1px 0 #4b4b4b, 1px -1px 0 #4b4b4b, 1px 0 0 #4b4b4b, 1px 1px 0 #4b4b4b, 0 1px 0 #4b4b4b, -1px 1px 0 #4b4b4b, -1px 0 0 #4b4b4b;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (performance): Consider using a more performant approach than text-shadow for hover effects

Multiple text-shadows can impact rendering performance. Consider using alternative approaches like border or box-shadow for the hover effect.

Suggested change
.iconHoverEffect:hover {
color: var(--hoverHighlight) !important;
cursor: pointer;
border-color: var(--hoverHighlight) !important;
text-shadow: -1px -1px 0 #4b4b4b, 0 -1px 0 #4b4b4b, 1px -1px 0 #4b4b4b, 1px 0 0 #4b4b4b, 1px 1px 0 #4b4b4b, 0 1px 0 #4b4b4b, -1px 1px 0 #4b4b4b, -1px 0 0 #4b4b4b;
}
.iconHoverEffect:hover {
color: var(--hoverHighlight) !important;
cursor: pointer;
border-color: var(--hoverHighlight) !important;
box-shadow: 0 0 0 1px #4b4b4b;
}

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