Skip to content

fix: move away from position fixed #1307

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Mar 29, 2025

Summary

Closes: #1305

This PR is migrating the top nav, sidebar and side navigation away from position: fixed, which is causing positioning issue when the page has an overflow hidden.

Position fixed is not really needed for these elements, and same results can be achieved using sticky, which also dispense the fact to re-compute all elements position with a translateX on sidebar opening on narrow screen.

This also allow footer elements to not overlap:

Screenshot 2025-04-04 at 03 34 26

Also fix an issue on the space creation page where the create button is larger than the content on some screen size

Screenshot 2025-04-04 at 03 38 15

Sidebar navigation is now scrollable when needed

Screen.Recording.2025-04-04.at.03.27.09.mov

How to test

  1. Open the app on a wide screen (>1900px)
  2. Open any modal
  3. All the content should remain fixed, the top nav and footer icons should not slide right by 15px.
  4. Everything else should works as before

@wa0x6e wa0x6e requested a review from Copilot March 29, 2025 23:08
Copy link
Contributor

@Copilot 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 updates the UI layout by replacing fixed positioning in headers, modals, and footers with sticky or absolute positioning to improve responsiveness and layout behavior. Key changes include updates to component wrappers and navigation elements and a refactor of modal styling with Tailwind's @apply utilities.

Reviewed Changes

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

Show a summary per file
File Description
apps/ui/src/views/Space/Editor.vue Removed conditional classes from UiTopnav and adjusted button and container classes.
apps/ui/src/components/Ui/Topnav.vue Replaced fixed header with a sticky layout and updated padding/margin classes.
apps/ui/src/components/Ui/Modal.vue Refactored modal styling by removing fixed positioning and applying scoped Tailwind classes.
apps/ui/src/components/Ui/Backdrop.vue Added a new backdrop component using a button element with Tailwind classes.
apps/ui/src/components/Layout/App.vue Updated layout structure to use flexbox for a more dynamic sidebar and main content area.
apps/ui/src/components/App/Topnav.vue Simplified the topnav template by removing unused div wrappers and conditionally rendering Breadcrumb.
apps/ui/src/components/App/Sidebar.vue Changed sidebar container classes for a fixed width and sticky inner layout.
apps/ui/src/components/App/Notifications.vue Adjusted notification positioning from fixed to absolute with updated spacing.
apps/ui/src/components/App/Nav.vue Modified navigation layout with a new sticky container for the Breadcrumb and navigation links.
apps/ui/src/components/App/Footer.vue Updated the footer’s position and link implementation from a fixed tag to an AppLink.
apps/ui/src/components/App/BottomNav.vue Added explicit height and flex styling to the bottom navigation for consistency.
Comments suppressed due to low confidence (1)

apps/ui/src/components/App/Topnav.vue:114

  • [nitpick] This empty div appears unnecessary and might add unwanted markup; consider removing it to simplify the template.
<div></div>

@@ -0,0 +1,3 @@
<template>
<button type="button" class="absolute inset-0 z-[99] bg-[black]/40" />
Copy link
Preview

Copilot AI Mar 29, 2025

Choose a reason for hiding this comment

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

The backdrop button is missing an accessible label (e.g., aria-label) which could affect accessibility; adding one is recommended.

Suggested change
<button type="button" class="absolute inset-0 z-[99] bg-[black]/40" />
<button type="button" class="absolute inset-0 z-[99] bg-[black]/40" aria-label="Backdrop" />

Copilot uses AI. Check for mistakes.

@wa0x6e wa0x6e marked this pull request as ready for review April 3, 2025 23:40
@wa0x6e wa0x6e requested a review from ChaituVR April 3, 2025 23:41
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.

ui: opening a modal is shifting position: fixed element
1 participant