Skip to content

Fix 'Add Initiative' modal positioning#733

Merged
4tal merged 1 commit into4tals:mainfrom
4tala:fix/modal-centering
Feb 8, 2026
Merged

Fix 'Add Initiative' modal positioning#733
4tal merged 1 commit into4tals:mainfrom
4tala:fix/modal-centering

Conversation

@4tala
Copy link
Copy Markdown
Contributor

@4tala 4tala commented Feb 8, 2026

Problem

The 'Add Initiative' (הוסיפו יוזמה) modal was opening off-screen, with the form content cut off at the top.

Solution

Changed the modal centering approach from fixed positioning with viewport percentages to flexbox centering:

.addSiteForm {
  position: fixed;
  inset: 0;
  display: flex;
  align-items: center;
  justify-content: center;
}

This ensures the modal is always properly centered regardless of sticky headers/footers or viewport size.

Changes

  • Modal container: Flexbox centering instead of top/bottom/left/right percentages
  • Backdrop: Stronger blur (4px) and darker overlay for better focus
  • Form container: White background, rounded corners, proper shadow
  • Close button: Centered at bottom, blue theme, hover state
  • X button: Circular with hover effect, positioned top-right

Before/After

Before: Modal positioned with top: 5vh; bottom: 5vh; left: 5vw; right: 5vw — caused content to be cut off

After: Modal centered with flexbox — always visible in viewport center

@netlify
Copy link
Copy Markdown

netlify bot commented Feb 8, 2026

Deploy Preview for linksforisrael ready!

Name Link
🔨 Latest commit 8fc98b1
🔍 Latest deploy log https://app.netlify.com/projects/linksforisrael/deploys/69889422f8d5ab0008f6aeb2
😎 Deploy Preview https://deploy-preview-733--linksforisrael.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix 'Add Initiative' modal positioning and improve styling

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Fixed modal positioning using flexbox centering instead of viewport percentages
• Improved backdrop blur effect and added dark overlay for better focus
• Redesigned close button with blue theme and centered placement
• Enhanced X button with circular styling and hover effects
• Better responsive height handling with max-width constraints
Diagram
flowchart LR
  A["Modal Container<br/>Fixed positioning"] -->|"Apply flexbox<br/>centering"| B["Centered Modal<br/>Always visible"]
  C["Backdrop<br/>Minimal blur"] -->|"Enhance blur<br/>& overlay"| D["Better Focus<br/>Dark backdrop"]
  E["Close Button<br/>Red, absolute"] -->|"Redesign styling<br/>& placement"| F["Blue button<br/>Centered"]
  G["X Button<br/>Plain text"] -->|"Add circular<br/>styling"| H["Circular button<br/>With hover"]
Loading

Grey Divider

File Changes

1. app/components/AddSite/AddSite.module.scss 🐞 Bug fix +57/-30

Modal centering and styling improvements

• Changed .addSiteForm from fixed positioning with top/left/right/bottom to flexbox centering
 with inset: 0 and display: flex
• Updated .formContainerBackdrop to use position: absolute with stronger blur(4px) and dark
 overlay rgba(0, 0, 0, 0.5)
• Redesigned .formContainer from fixed positioning to relative positioning with responsive width
 (90vw, max-width: 400px) and height constraints
• Enhanced .closeFormButton with blue background (#3182ce), centered placement (`margin: 12px
 auto`), and hover state
• Improved .closeFormXButton with circular styling (border-radius: 50%), background color, and
 hover effects

app/components/AddSite/AddSite.module.scss


Grey Divider

Qodo Logo

- Use flexbox centering on parent for reliable modal positioning
- Fix close button styling to match dark theme
- Add circular X button with hover effect
@4tala 4tala force-pushed the fix/modal-centering branch from 32c5f71 to 8fc98b1 Compare February 8, 2026 13:48
@qodo-code-review
Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Avoid transition: all 🐞 Bug ➹ Performance
Description
• The PR changes button transitions from targeting a specific property to transition: all, which
  can cause unintended animations when any other property changes in the future.
• This is harder to maintain/debug and can trigger unnecessary repaint/layout work compared to
  transitioning only the properties that actually change (e.g., background-color, color).
Code

app/components/AddSite/AddSite.module.scss[R18-29]

+	transition: all 0.2s ease;
+	display: block;
+	margin: 12px auto;
+	font-size: 14px;
+	font-weight: 500;
+	cursor: pointer;
+	border: none;
+	flex-shrink: 0;
+
+	&:hover {
+		background-color: #2c5282;
+	}
Evidence
The updated .closeFormButton now uses transition: all while only background-color changes on
hover; similarly, .closeFormXButton also uses transition: all. Limiting transitions to the
properties that actually change avoids accidental animations if additional properties are
added/modified later.

app/components/AddSite/AddSite.module.scss[12-30]
app/components/AddSite/AddSite.module.scss[32-54]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`transition: all` was introduced for `.closeFormButton` and `.closeFormXButton`. This can cause unintended animations when additional CSS properties are changed later, and can add unnecessary repaint/layout work.

### Issue Context
Both buttons only change a small set of properties on `:hover` (background/background-color and text color). The transition should target those properties explicitly.

### Fix Focus Areas
- app/components/AddSite/AddSite.module.scss[12-54]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@4tal 4tal merged commit 0a3ac47 into 4tals:main Feb 8, 2026
5 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