fix(DialInNumber,InviteByEmailSection):Copy Button Ui

[HazemAbdo]

Fixed (Fix invite People copy number UI) #14551
New copy number button behavior:

• It has a tooltip that shows what it does
• when clicked the icon converted to IconCheck for 2500ms like other copy icons then returns back to the IconCopy
  New Copy Invitation button behavior:
• when clicked the icon converted to IconCheck for 2500ms like other copy icons then returns back to the IconCopy

copy_new.mp4

[jitsi-jenkins]

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

[HazemAbdo]

I have already signed the CLA

[horymury]

Jenkins please test this please

[saghul]

Jenkins please test this please.

[HazemAbdo]

What can I do to pass this check as it said that 0 test failed @horymury @saghul

[damencho]

I'm working on fixing the pr-tests, so hold on. I will restart the job once it is fixed.

[horymury]

@HazemAbdo the PR looks good overall, just a small nit.
Since you're using setTimeout in the components, it would be good to guard against component unmounted when calling the timeout handler (case when the component unmounts before the timeout callback is called), otherwise we might get errors for attempting to change state of an unmounted component. In my tests I saw that once mounted they do not unmount so I could not reproduce the error myself.

You have an example for checking mount state for a class component here
react/features/recording/components/LiveStream/AbstractStartLiveStreamDialog.ts

[horymury]

@HazemAbdo the PR looks good overall, just a small nit. Since you're using setTimeout in the components, it would be good to guard against component unmounted when calling the timeout handler (case when the component unmounts before the timeout callback is called), otherwise we might get errors for attempting to change state of an unmounted component. In my tests I saw that once mounted they do not unmount so I could not reproduce the error myself.

You have an example for checking mount state for a class component here react/features/recording/components/LiveStream/AbstractStartLiveStreamDialog.ts

We have an example for exactly your use case for functional components :)
react/features/base/buttons/CopyButton.web.tsx

[HazemAbdo]

@horymury I think I can rewrite this functionality using react/features/base/buttons/CopyButton.web.tsx and right then I will ask for your review.I was thinking in rewriting the react/features/invite/components/add-people-dialog/web/DialInNumber.tsx component as a functional component to make it like react/features/invite/components/add-people-dialog/web/InviteByEmailSection.tsx and react/features/invite/components/add-people-dialog/web/CopyMeetingLinkSection.tsx.

[horymury]

@horymury I think I can rewrite this functionality using react/features/base/buttons/CopyButton.web.tsx and right then I will ask for your review.I was thinking in rewriting the react/features/invite/components/add-people-dialog/web/DialInNumber.tsx component as a functional component to make it like react/features/invite/components/add-people-dialog/web/InviteByEmailSection.tsx and react/features/invite/components/add-people-dialog/web/CopyMeetingLinkSection.tsx.

@HazemAbdo if you can convert DialInNumber.tsx to a functional component would be greatly appreciated :) Also please feel free to use ES6 syntax if you prefer on the conversion. Thank you.

[HazemAbdo]

@horymury I rewrote react/features/invite/components/add-people-dialog/web/InviteByEmailSection.tsx and react/features/invite/components/add-people-dialog/web/DialInNumber.tsx using react/features/base/buttons/CopyButton.web.tsx and make some changes on CopyButton to make it fit with this two components I also convert DialInNumber to function component and add showSuccessNotification to be shown after clicking on it.

dial_copy_functional.mp4

[horymury]

@HazemAbdo appreciate the effort in reusing the existing CopyButton, but at a second look I think that repurposing it for the two new cases complicates things a bit too much, for example the use of !important which we don't quite condone, plus the fact that in order for the copy button to be inline with the other 4 buttons there(invite by google, yahoo etc buttons) too many hackish things need to be done. You can already see that in your attempt, the button is not quite same size with the rest, if you reduce the width and height to be same size with the rest, icon gets misaligned (icon has a margin of 8px, the icon container is not center-aligned due to some padding - it's skewed to the right) and other small styling changes which I might have missed as well, not to mention the addition of props and acomodation for no-text mode which all add to unneeded complexity.

Also, on the DialInNumber component it changes a bit the previous design of the button.

So I'd suggest you revert to the previous iteration with treating the isMounted case and with keeping the refactor of the DialInNumber as a functional component,which we are grateful you did :)

It might have been a misunderstanding, in my previous message I was referring to CopyButton.web.tsx as an example for checking isMounted not suggesting to be reused, apologies for not being more specific.

Thank you

[HazemAbdo]

@horymury No problem at all I will do these changes and update the PR with them. I thought that these changes of CopyButton might not very straight forward and make it a little bit more complicated and change it is main use case and they might not be the best solution for our problem.Thank you for your valuable feedback.

[HazemAbdo]

@horymury I rewrote react/features/invite/components/add-people-dialog/web/InviteByEmailSection.tsx and react/features/invite/components/add-people-dialog/web/DialInNumber.tsx but this time without using CopyButton and reset this CopyButton component to its original implelemntation.Thank you in advance and I am really sorry if this PR took a lot of time from you as I asked you many times for your review.

dialIn_v3.mp4

[horymury]

Jenkins please test this please.

[saghul]

Jenkins please test this please.