Skip to content
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

fix(CallView): light improvement #12300

Merged
merged 5 commits into from
May 13, 2024
Merged

fix(CallView): light improvement #12300

merged 5 commits into from
May 13, 2024

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented May 7, 2024

☑️ Resolves

  • When joining a call in group conversation, it is set to grid view even for "connecting" view which makes the tile looks weird. I enforce a speaker view when connecting so to keep a consistent view for all joiners and to avoid having an expanded local video tile.
  • Stop following button lost restoring the previous view state because of some watchers enforcing the temporary view (speaker view). I omitted those as startPresentation already handles it.

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image

🚧 Tasks

  • ...

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • 🖥️ Tested with Desktop client or should not be risky for it
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@DorraJaouad DorraJaouad added this to the 💙 Next Major (30) milestone May 7, 2024
@DorraJaouad DorraJaouad requested review from ShGKme and Antreesy May 7, 2024 11:17
@DorraJaouad DorraJaouad self-assigned this May 7, 2024
src/components/CallView/CallView.vue Show resolved Hide resolved
src/components/CallView/CallView.vue Show resolved Hide resolved
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's aligned with tests, let's go with it for now.
I'd still consider to discuss all possible variants and their outputs as follow-up (like with a table we did for scrolling)

ShGKme
ShGKme previously approved these changes May 8, 2024
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that looks weird now, is that on the empty call view the switch does nothing. You click on "Grid view" or "Speaker view" and nothing got changed. But I don't see any alternatives.

image

@ShGKme
Copy link
Contributor

ShGKme commented May 8, 2024

  1. On the call, start following somebody and open stripe
  2. Start screensharing from another participant
  3. Stop screensharing
  4. No you are on the grid view instead of the previous "Following + open stripe"

Is that expected?

Before it returned to the same state, you were before presentation.

@Antreesy
Copy link
Contributor

Antreesy commented May 8, 2024

But I don't see any alternatives.

Hide an option, while there's nothing to place on grid?

@ShGKme ShGKme dismissed their stale review May 8, 2024 15:29

State is not restored on return from presentation

@DorraJaouad DorraJaouad requested review from Antreesy and ShGKme May 8, 2024 16:52
@ShGKme
Copy link
Contributor

ShGKme commented May 9, 2024

Not sure it is related, but caught an exception:

TypeError: Cannot read properties of null (reading 'attributes')
    at VueComponent.shouldShowPresenterOverlay

on this line

return this.shownRemoteScreenCallParticipantModel.attributes.videoAvailable || this.isModelWithVideo(this.shownRemoteScreenCallParticipantModel)

src/components/CallView/CallView.vue Show resolved Hide resolved
src/store/callViewStore.js Outdated Show resolved Hide resolved
@DorraJaouad
Copy link
Contributor Author

TypeError: Cannot read properties of null (reading 'attributes')
at VueComponent.shouldShowPresenterOverlay

This was unrelated, but fixed as it is involved in "light improvement" :p

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last nitpick from me, can't tell what else we could enhance atm. Behaves much better, in my opinion

src/components/TopBar/TopBarMenu.vue Outdated Show resolved Hide resolved
…he previous isGrid last state accurate.

Signed-off-by: DorraJaouad <[email protected]>
…ences that are set during presentation

Signed-off-by: DorraJaouad <[email protected]>
…or selected video. If both of happen at the same time (e.g: someone shared a screen and you are focusing of a video), closing one of them should not trigger restore to the previous state which will cancel the other.

Signed-off-by: DorraJaouad <[email protected]>
@Antreesy Antreesy enabled auto-merge May 13, 2024 13:04
@Antreesy Antreesy merged commit 133a7ca into main May 13, 2024
46 checks passed
@Antreesy Antreesy deleted the fix/call-view branch May 13, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video/people layout of group calls or conversations shared by link looks off initially
3 participants