-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add edit icon #5346
Add edit icon #5346
Conversation
@mountiny I have added a tooltip as well. This PR is ready. Adding a tooltip to this icon was not straightforward and it was left out in the issue which was supposed to add that. (please read till the end #4499 (comment) to know more). I would like to request to submit another PR to add tooltips to other parts of the app that were left deliberately due to the Tooltip component's limitation. I have added the code to remove the limitation which is additional work for this job. It would be a way of getting compensation for the additional work as well as fixing that problem and not just a bonus. Let me know what do you think about this? |
@parasharrajat I like that idea. To recap, you have fixed the problem with positioning tooltips over elements which are absolutely positioned? The new issue would be to add the tooltips to all the elements left out due to this limitation? If so, feel free to create a new issue for this and propose a solution there. You can ping me here and I can triage it as I already have a context for it. Would you consider including this idea from Shawn in your new issue too #4499 (comment)? Or do you reckon that would be more substantial/complicated change? |
You are correct with first paragraph. I see, Shawn's Idea is amazing. I would love to invest some time into it. I have a generic plan on How we can do that Let me try that and see if that works. |
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.
Great job on this one! Code looks good to me 🚀
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
Fixed Issues
$ #5326
Tests | QA Steps
Edit workspace
.Tested On
Screenshots