element-call icon indicating copy to clipboard operation
element-call copied to clipboard

Add testID tags for new UI elements

Open michaelkaye opened this issue 2 years ago • 5 comments

Recently we've had some new elements which need testid tags to be able to navigate through:

Add a testid tag to each SVG so we can detect if they're on-screen or not (to determine if the button is muted or not) Add testid tags to each tab in the settings menu so we can switch between them easily. Remaining additions were previously existing and seem to have got lost in refactors.

michaelkaye avatar Aug 30 '23 16:08 michaelkaye

Codecov Report

Patch coverage has no change and project coverage change: +0.16% :tada:

Comparison is base (e635740) 26.04% compared to head (a3535dd) 26.21%. Report is 15 commits behind head on livekit.

Additional details and impacted files
@@             Coverage Diff             @@
##           livekit    #1363      +/-   ##
===========================================
+ Coverage    26.04%   26.21%   +0.16%     
===========================================
  Files           50       50              
  Lines         2077     2064      -13     
  Branches       336      336              
===========================================
  Hits           541      541              
+ Misses        1499     1486      -13     
  Partials        37       37              
Flag Coverage Δ
unittests 26.21% <ø> (+0.16%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Aug 30 '23 17:08 codecov-commenter

So the icons have changed, they have a data-testid in them now. It's the easiest way (especially since we have a SVG rather than a PNG or JPG) to know when a certain image is displayed on screen.

<div class="_previewButtons_a755q_56"><button class="_toolbarButton_nw1rl_18 _off_nw1rl_82" data-testid="preview_mute" type="button" style="user-select: none;">
<svg data-testid="icon_audiomute" width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M...Z" fill="white"></path></svg>
</button>...</div>

vs

<div class="_previewButtons_a755q_56"><button class="_toolbarButton_nw1rl_18" data-testid="preview_mute" type="button" style="user-select: none;">
<svg data-testid="icon_audio" width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M...Z" fill="#ffffff"></path></svg></button>...</div>

For if the mute is pressed or not. The "preview_mute" testid is what we choose to click on. The SVG changes data-testid between icon_audio and icon_audiomute.

If we don't go this way we could either try to hook into what <button class="_toolbarButton_nw1rl_18" /> vs <button class="_toolbarButton_nw1rl_18 _off_nw1rl_82" /> is, or possibly have the data-testid of the button vary based on whether it's trying to show mute or unmute.

michaelkaye avatar Aug 31 '23 10:08 michaelkaye

Thinking about it a bit wider, I'm not sure how these are generated and if we're going to run into any problems if we reskin the app in some automated fashion.

It'll definitely fail if it's removed because the check for mute status in my code checks for both and returns whichever one it sees so we'll notice if we run into it.

michaelkaye avatar Aug 31 '23 10:08 michaelkaye

Looks like i might need to work a bit on detecting the icon muting or not with these changes.

michaelkaye avatar Sep 15 '23 10:09 michaelkaye

@dbkr : this has moved on a few times since you reviewed; do you want another review of it; or are you happy for me to merge it as is?

michaelkaye avatar Sep 28 '23 17:09 michaelkaye

This needs a rebase.

toger5 avatar Jul 17 '24 08:07 toger5

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Sep 06 '24 08:09 CLAassistant

@michaelkaye this is definitly useful. Are you still planning to work on it and resolve the conflicts?

toger5 avatar Sep 09 '24 12:09 toger5

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

I don't intend to do this CLA; I was an employee when this was written, so if anyone wants to pick it up and work with it, it should be fine without the agreement.

I imagine the testID tags are likely out of date by now, as the app has evolved the UI, though maybe the image changes are useful as a strategy? Would definitely suggest starting from scratch as the better way to approach this sort of change.

I'm not planning on doing more on this PR, will close it now.

michaelkaye avatar Sep 09 '24 12:09 michaelkaye