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

Fixing Css logic to correctly show rate limit banners in the correct place #6464

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arafatkatze
Copy link
Collaborator

Linear issue

image

Currently the UI for displaying the rate limit being exceeded is bad because it shows up in the wrong place. While I am not "stopping" the user from clicking on new chat I think showing the go pro banner at the right place and then making every chat error out is a good enough mechanism to remind people to go pro.

So here's what the new banners will look like after the PR

Jetbrains

image

Vscode

image

Test plan

You can test this by going to rate-limit-ui-fix-testing branch and then running it in the vscode debugger and use a non-pro account with the dotcom instance and you will ALWAYS see the banner

Changelog

Copy link
Contributor

@vovakulikov vovakulikov left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change, if we want to move this button we should change the appearance of this as well I guess

But I also think we shouldn't move it because in the banner we already have the "upgrade" button which does the same thing as "go pro" button

By the way, I noticed that for some reason "go pro" isn't a button for some reason (but div with role=button), I would make it a common button to avoid handling tab index (and I think currently keyboard shortcuts like clicking by space or enter don't work on this div-like button)

height: 100px;
z-index: 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

We (usually) don't use z-index greater than 0 or 1, I would try to solve this with isolation instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good pointer, I will respond after getting the opinion of the whole situation from @toolmantim and @taiyab

top: 0;
right: 0;
overflow: hidden;
overflow: visible;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this container has 100px, but with overflow visible, it doesn't make much sense to have a fixed height because, with the visible value, it will overflow anyway.

@arafatkatze
Copy link
Collaborator Author

“If we want to move this button, we should change the appearance of this as well, I guess.”

You’re right that the appearance should align with the button’s placement and functionality. If we move the “Go Pro” button, we should ensure its design and positioning are intuitive and visually consistent with the rest of the UI. However, the current fix wasn’t focused on moving the button but on ensuring that the banner displays fully and correctly.

@arafatkatze
Copy link
Collaborator Author

arafatkatze commented Dec 26, 2024

I think we shouldn’t move it because the banner already has the ‘upgrade’ button which does the same thing

This is a valid point. If the “Go Pro” button duplicates the functionality of the “Upgrade” button, we should consolidate these actions to avoid redundancy. It would make sense to either:
1. Remove the “Go Pro” button entirely if it’s not necessary, or
2. Clearly differentiate the functionality if both buttons serve slightly different purposes.

CC @toolmantim @taiyab if you can offer commentary on this it would be nice so that we have definitive idea of how the design for this should proceed and then I can make the necessary changes.

@arafatkatze arafatkatze requested a review from taiyab December 26, 2024 13:10
@toolmantim
Copy link
Contributor

Thanks for looking into this @arafatkatze!

The intended display is meant to be more like what’s in the before screenshot (like a little band wrapped around the corner of the cell, with centered text, corners cropped), but positioned on the system row that has the upgrade required notice (not over the input cell).

Are we simply missing a position relative on each cell, so the absolutely positioned band is absolute relative to the cell?

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.

3 participants