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

Color of menu buttons should adhere to Main Event Color #210

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

lcduong
Copy link
Contributor

@lcduong lcduong commented Sep 30, 2024

…color should be available for scroll over

This PR closes/references issue #206 . It does so by use the main color to set the button color

image

How has this been tested?

Checklist

  • I have added tests to cover my changes.

Summary by Sourcery

Update the menu button colors to adhere to the main event color, enhancing the visual consistency of the interface. Introduce a function and mixin to dynamically adjust text color based on background brightness for improved readability.

Enhancements:

  • Implement a function to determine if a color is dark and a mixin to set contrast color for better readability.
  • Update the styling of menu buttons to use the main event color, ensuring consistent theming across the interface.

Copy link

sourcery-ai bot commented Sep 30, 2024

Reviewer's Guide by Sourcery

This pull request implements changes to the color scheme of menu buttons to adhere to the main event color. It introduces new SCSS functions and mixins to handle color contrast and applies these to the button styles in the agenda view.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant B as Button
    participant S as SCSS
    U->>B: Hover over button
    B->>S: Request hover styles
    S->>S: Call is-dark() function
    S->>S: Determine text color with contrast-color mixin
    S-->>B: Apply hover styles (background and text color)
    B-->>U: Display updated button appearance
Loading

File-Level Changes

Change Details Files
Implement dynamic color contrast for buttons
  • Add is-dark() function to determine if a color is dark
  • Create contrast-color mixin to set text color based on background
  • Apply contrast-color mixin to button hover and active states
src/pretalx/static/agenda/scss/_agenda.scss
Update button styles to use main event color
  • Set button color and border-color to $brand-primary
  • Apply $brand-primary as background color on hover and active states
  • Update summary element color to use $brand-primary
src/pretalx/static/agenda/scss/_agenda.scss
Refactor schedule navigation styles
  • Adjust indentation for better readability
  • Remove unnecessary active class styles
src/pretalx/static/agenda/scss/_agenda.scss

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.

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

Copy link

@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 @lcduong - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider using a standardized method for determining color contrast instead of magic numbers in the brightness calculation.
  • Look into extracting common styles for &:hover and &.active states into a separate mixin to reduce code duplication.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues 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.

@@ -43,20 +43,50 @@ header {
}
}

@function is-dark($color) {
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider using a standardized method for calculating contrast ratios

While the current implementation is a good start, using a method aligned with WCAG guidelines could provide more accurate results across a wider range of colors.

@function is-dark($color) {
  $luminance: (0.2126 * red($color) + 0.7152 * green($color) + 0.0722 * blue($color)) / 255;
  @return $luminance < 0.5;
}

Comment on lines 77 to 84
.btn-outline-success {
color: $brand-primary;
border-color: $brand-primary;

&:hover {
background-color: $brand-primary;
@include contrast-color($brand-primary);
}
Copy link

Choose a reason for hiding this comment

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

suggestion: Reduce duplication in hover and active states

Consider using a placeholder selector or a mixin to reduce the repetition of background-color and @include contrast-color for both :hover and .active states. This would improve maintainability and reduce the risk of inconsistencies if these styles need to be updated in the future.

    .btn-outline-success {
      color: $brand-primary;
      border-color: $brand-primary;

      @mixin active-state {
        background-color: $brand-primary;
        @include contrast-color($brand-primary);
      }

      &:hover, &.active {
        @include active-state;
      }

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

When the color is dark we need the font color to switch to white to have a good readable contrast.

@lcduong
Copy link
Contributor Author

lcduong commented Oct 1, 2024

Hi @mariobehling, updated to change font color when background color is dard

image

@mariobehling mariobehling merged commit 37d500c into fossasia:development Oct 1, 2024
4 checks passed
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.

2 participants