-
Notifications
You must be signed in to change notification settings - Fork 156
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
Feature/397 #1036
Feature/397 #1036
Conversation
I have to Apply condition based Title for PluginDocumentSettingPanel and for that I had created const and added that const in title as below but somehow it is not working. Can you please check once?
|
I have integrated this feature in Modal but Need to setup design so can you please assign one designer for the same? |
@peterwilsoncc , You can ignore this as its been resolved. |
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.
We can hold the review. I raised a question on issue #397 (comment)
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.
Thanks for working on this @faisal-alvi. Great work here, I have added 2 minor comments. Also, for me the "Unlink" button is disabled and I am also not able to click (X) on the popup. Could you please help to check?
Thank you.
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.
Tests well 👍 , Approving not merging yet.
@jeffpaul following up on the comment by Ravinder here - #397 (comment)
cc: @dkotter as Jeff is on PTO.
If it's cool with you then it's good to go.
I think I agree that these icons could be clearer but I also don't have a great suggestion in mind. I think we're fine to ship this as-is and we can iterate on it later. That said, I see there's a couple comments by @iamdharmesh that need addressed and also seeing a bunch of eslint issues we should clean up. Once those are taken care of, I'm good to get this merged in |
Co-authored-by: Dharmesh Patel <[email protected]>
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.
Thanks for the changes @faisal-alvi @Sidsector9. LGTM.
Just added a minor suggestion but nothing blocker.
Co-authored-by: Dharmesh Patel <[email protected]>
Description of the Change
Closes #397
How to test the Change
Changelog Entry
Credits
Props @roshniahuja @faisal-alvi
Checklist: