-
Notifications
You must be signed in to change notification settings - Fork 771
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
Remove v11 banner #4169
Remove v11 banner #4169
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Thy! Looks good 🔥
Just one minor point and one doubt:
- I think we missed removing the v11 inline notification callout in the Popover component.
![Screenshot 2024-07-11 at 11 53 34 AM](https://private-user-images.githubusercontent.com/158570656/347703471-d4d8cf91-6e79-4192-bccc-c6dba155d17d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDExNTgsIm5iZiI6MTcyNTYwMDg1OCwicGF0aCI6Ii8xNTg1NzA2NTYvMzQ3NzAzNDcxLWQ0ZDhjZjkxLTZlNzktNDE5Mi1iY2NjLWM2ZGJhMTU1ZDE3ZC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTA2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkwNlQwNTM0MThaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT03NGI4YjI0YjU3Njk5NTE2ZTgxNDY1N2Y4M2E4YmI4ZDhjNzI1NWVhZmMxMDIzYzMyZDljZWNiNmQ1Mjk5ZjU4JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.QRQpKI-uOdlcGerG8MpCzwphTz2KkTaCNxrM-NPKrBQ)
(Doubt:)
- I saw "Tabs" mentioned in the issue, but I can't find the v11 callout under that component.
- There's only one inline notification under the Tab usage component. I'm not sure if the "Updates for grid alignment" are available now or not. If so, we can update that message with the latest changes. (This can be handled separately by creating another issue, but I just wanted to confirmed that since I noticed it while reviewing this PR.)
![Screenshot 2024-07-11 at 11 53 11 AM](https://private-user-images.githubusercontent.com/158570656/347703369-24f4fe4a-d6c8-46d7-9245-29def2f8ec62.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDExNTgsIm5iZiI6MTcyNTYwMDg1OCwicGF0aCI6Ii8xNTg1NzA2NTYvMzQ3NzAzMzY5LTI0ZjRmZTRhLWQ2YzgtNDZkNy05MjQ1LTI5ZGVmMmY4ZWM2Mi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTA2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkwNlQwNTM0MThaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT03ODQwYTU0YzBiNzAzMjA2N2UyODBlMmFiNmM3Y2RkOWU3YTlhNDgzNmFlZGVmMWU0YTE0NTk3ZDUxMGNkNTc2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.c1ziXP2vJIje1_iLXoQAhr2zUqIhGFdDOqfSv8lG9MA)
@Kritvi-bhatia17 @thyhmdo Tracey removed the Tabs v11 notification at the top of the page in her recent Tab component docs PR that was merged, so that is why the banner is not there. I accidentally marked her in the issue as having removed the Notification component v11 banner, but that was incorrect because it was the Tabs component. I changed it back in the issue to be accurate. So this looks fine to me.
I would keep this notification about grid-aware tabs for now. We need to follow up with Alison to see if grid-aware tabs are in the code yet. I know some issues and PRs have been closed around this enhancement, but it would be good to double-check with her before removing it. |
Oh sounds good @laurenmrice, thanks for clarifying. ❤️ |
@Kritvi-bhatia17 Yes, Popover still needs to have its banner removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thy!! 🔥
Closes #4112
Removed v11 notifications
Additional changes
-- Kept the migration link in the Usage tab
-- Removed the migration note of v10