Skip to content

fix: Fixed 10080 settings sidebar scrolling issue#10471

Merged
emrysal merged 4 commits into
calcom:mainfrom
bhargavtenali:fix/top-banner-z-index
Jul 31, 2023
Merged

fix: Fixed 10080 settings sidebar scrolling issue#10471
emrysal merged 4 commits into
calcom:mainfrom
bhargavtenali:fix/top-banner-z-index

Conversation

@bhargavtenali

@bhargavtenali bhargavtenali commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

fixes #10080

Video Link : https://drive.google.com/file/d/1gMdWe0NhVEB6i2pXIzfTpelFNvQKGL5E/view?usp=sharing

Hi @Jaibles @PeerRich @Udit-takkar
Hope you are doing well.
After hours of debugging to find the exact root cause. This is what I found:
Issue and Fix Summary:

  1. The Issue [CAL-2151] Top banner has weird z-index position in settings #10080 is not because of z-index. Infact the sidebar & mainPage are having position Sticky elements with appropriate z-index. The topbar is a static element but child of Sticky Element.
  2. But the issue is because we havent given max-height and top property's values for settings SideBar.
  3. I had tested the same fix on mobile -> It works as expected
  4. I had used React.cloneElement using which we can pass an extra prop bannersHeight [top banner height] to props.SidebarContainer [settings Sidebar]
  5. I had used bannersHeight in props.SidebarContainer [settings Sidebar] for top and max-height
  6. I feel there is no need to change anything with topBar.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

When Any topBar is present (eg: Organisation Upgrade Banner) -> Go Settings Page -> scroll through the sidebar present in Settings Page
Video Link : https://drive.google.com/file/d/1gMdWe0NhVEB6i2pXIzfTpelFNvQKGL5E/view?usp=sharing

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

@vercel

vercel Bot commented Jul 30, 2023

Copy link
Copy Markdown

@bhargavtenali is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@vercel

vercel Bot commented Jul 30, 2023

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 31, 2023 6:31pm

@github-actions github-actions Bot added Medium priority Created by Linear-GitHub Sync ui area: UI, frontend, button, form, input labels Jul 30, 2023
@github-actions

github-actions Bot commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

Thank you for following the naming conventions! 🙏

@github-actions github-actions Bot added ⚡ Quick Wins A collection of quick wins/quick fixes that are less than 30 minutes of work ✅ good first issue Good for newcomers 🐛 bug Something isn't working labels Jul 30, 2023
@github-actions

github-actions Bot commented Jul 30, 2023

Copy link
Copy Markdown
Contributor

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@bhargavtenali bhargavtenali changed the title fixed 10080 settings sidebar scrolling issue fix: Fixed 10080 settings sidebar scrolling issue Jul 30, 2023
@Udit-takkar Udit-takkar self-requested a review July 30, 2023 21:29
Comment thread packages/features/settings/layouts/SettingsLayout.tsx
Comment thread packages/features/settings/layouts/SettingsLayout.tsx Outdated
Signed-off-by: Udit Takkar <udit.07814802719@cse.mait.ac.in>
Udit-takkar
Udit-takkar previously approved these changes Jul 31, 2023

@Udit-takkar Udit-takkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I fixed the type error

Comment thread packages/features/settings/layouts/SettingsLayout.tsx Outdated

@emrysal emrysal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-approving @Udit-takkar after fixing failing type error.

@emrysal emrysal enabled auto-merge (squash) July 31, 2023 18:22
@emrysal emrysal merged commit 12abca6 into calcom:main Jul 31, 2023
sean-brydon pushed a commit that referenced this pull request Aug 1, 2023
* fix: #10080 settings sidebar scrolling issue

* fix: type error

Signed-off-by: Udit Takkar <udit.07814802719@cse.mait.ac.in>

* Fixed type error

---------

Signed-off-by: Udit Takkar <udit.07814802719@cse.mait.ac.in>
Co-authored-by: Udit Takkar <udit.07814802719@cse.mait.ac.in>
Co-authored-by: Alex van Andel <me@alexvanandel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working ✅ good first issue Good for newcomers Medium priority Created by Linear-GitHub Sync ⚡ Quick Wins A collection of quick wins/quick fixes that are less than 30 minutes of work ui area: UI, frontend, button, form, input

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-2151] Top banner has weird z-index position in settings

3 participants