Skip to content

Conversation

@abdimo101
Copy link
Member

@abdimo101 abdimo101 commented Dec 17, 2025

Description

This PR updates the search bars max-width to 33% of the header while keeping the min-width unchanged

Motivation

Background on use case, changes needed

Fixes:

Please provide a list of the fixes implemented in this PR

  • Items added

Changes:

Please provide a list of the changes implemented by this PR

  • changes made

Tests included

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)

Documentation

  • swagger documentation updated [required]
  • official documentation updated [nice-to-have]

official documentation info

If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included

Backend version

  • Does it require a specific version of the backend
  • which version of the backend is required:

Summary by Sourcery

Enhancements:

  • Update search bar styling so it maintains a minimum width of 500px while capping its maximum width to one-third of the header.

@abdimo101 abdimo101 requested a review from a team as a code owner December 17, 2025 09:37
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 there - I've reviewed your changes - here's some feedback:

  • With width: 100%, min-width: 500px, and max-width: 33% on a flex item, there’s a risk of layout issues on smaller headers where 33% is less than 500px (the min-width will win and can cause overflow), so it’s worth double-checking the responsive behavior and possibly basing the sizing on flex-basis instead of width.
  • If the intent is to make the search bar at most one third of the header but never smaller than 500px, consider whether the container will always be wide enough (≥1500px); if not, you may want to relax the min-width or use a more flexible constraint (e.g., min(33%, 500px) or similar) depending on browser support.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- With `width: 100%`, `min-width: 500px`, and `max-width: 33%` on a flex item, there’s a risk of layout issues on smaller headers where 33% is less than 500px (the min-width will win and can cause overflow), so it’s worth double-checking the responsive behavior and possibly basing the sizing on flex-basis instead of width.
- If the intent is to make the search bar at most one third of the header but never smaller than 500px, consider whether the container will always be wide enough (≥1500px); if not, you may want to relax the `min-width` or use a more flexible constraint (e.g., `min(33%, 500px)` or similar) depending on browser support.

## Individual Comments

### Comment 1
<location> `src/app/shared/modules/dynamic-material-table/table/dynamic-mat-table.component.scss:373-374` </location>
<code_context>
     flex: 1 1 auto;
     width: 100%;
-    max-width: 500px;
+    min-width: 500px;
+    max-width: 33%;

     mat-form-field {
</code_context>

<issue_to_address>
**issue (bug_risk):** The min/max width combination is likely to cause layout issues and may not match the intended responsiveness.

With `flex: 1 1 auto`, combining `min-width: 500px` and `max-width: 33%` will cause horizontal overflow below 500px and make the 33% cap ineffective until ~1500px. This can also interfere with how sibling flex items wrap. Consider whether you meant `max-width: 500px` with a smaller `min-width`, or swapping to a percentage-based `min-width` and pixel-based `max-width` to keep the layout responsive and avoid overflow.
</issue_to_address>

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.

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