-
Notifications
You must be signed in to change notification settings - Fork 287
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
Allow setting tooltips #371
Conversation
Hi @NileshPatel17, could I get a review on this? It’s necessary to fulfill a business requirement, and we’re currently working it around by referencing a package built locally from my fork, but it results in extra work for each developer and a continuously dirty local Git repo in the dependent project (I can’t commit the change to |
@jtotht will take a look, and if all looks good, will merge and publish new version. |
Hi @NileshPatel17 , have you taken a look at this Pull Request already, this is an important upgrade that will benefit many users. Thanks in advance! |
@guerreda1095 added review comment, did not hear it back.. please fix it, and i will merge.. |
@NileshPatel17 I'm not seeing any new pending comment to review, @jtotht are you able to see Nilesh's comments? |
No, I don’t see any review comments either, neither here nor in the diff view. Are you sure @NileshPatel17 that you actually published them? |
I wonder, why you do not see review comment. here is the link, https://github.com/NileshPatel17/ng-multiselect-dropdown/pull/371/files#diff-32a865e149b5c9b57ca8fa46f97a3499a97842afb9c36b5b076c9968e0fc3505 once you fix it, i will merge the code and publish new version. |
I also wonder (I haven’t encountered any such issues on GitHub before), but I still don’t see it – I see that your link leads me to the diff of |
Or if you feel like it, you can do the changes yourself and push them to my fork so that they become part of this PR – as a maintainer, I’ve granted you push right to the |
@jtotht please make below change. rest all is good. tooltip will always be string, can not be number. replace this
with
|
The new `tooltip` field of the ListItem class makes it possible to add extended information as a tooltip. Also refactor the component a bit so that the new field needs to be handled only at one place. Fixes NileshPatel17#317
Good point, fixed. |
@jtotht published new version 0.3.9. Pleaser test and let me know if it works or not. its not working for me at least with angular 14 I get this error
|
Fix an actually failing test written by me in NileshPatel17#371. Also fix some tests (one introduced by me and some others that have already been there) that weren’t marked as failures, but threw TypeErrors during the test runs.
Thanks for merging and publishing! I have no idea what’s going on with the StackBlitz example – I can’t go into the actual implementation files of However, I do know why the CI job failed, so I submitted a PR to fix it: #379. (It would’ve been better if CI ran earlier, and I could fix the failure in this PR, but better late than never.) |
Hi team |
You requested version |
@NileshPatel17 did you remove this feature in latest version [1.0.0] ? |
The new
tooltip
field of the ListItem class makes it possible to add extended information as a tooltip. Also refactor the component a bit so that the new field needs to be handled only at one place.Fixes #317