Fixing Css logic to correctly show rate limit banners in the correct place #6464
Conversation
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
We (usually) don't use z-index greater than 0 or 1, I would try to solve this with isolation instead.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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. |
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: 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. |
|
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? |
arafatkatze
force-pushed
the
rate-limit-ui-fix
branch
from
3848aed to
3263dac
Compare
| onKeyDown={() => onButtonClick('upgrade', 'upgrade')} | ||
| > | ||
| Go Pro | ||
| {canUpgrade && ( |
There was a problem hiding this comment.
Had to change the ordering here to make this possible
Linear issue
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.
Positioning Fixes:
Styling Improvements:
Improved spacing and typography
Visual Refinements:
So here's what the new banners will look like after the PR
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