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

feat(TwinMaker): Setting up sticky video controls #190

Merged
merged 1 commit into from
May 23, 2023
Merged

Conversation

mumanity
Copy link
Collaborator

@mumanity mumanity commented May 5, 2023

What this PR does / why we need it:

  • Improves video player user experience.

Which issue(s) this PR fixes:

  • Applies sticky placement to the video-upload-request-ui block.

Special notes for your reviewer:

grafana.video.player.mov

@mumanity mumanity requested a review from a team as a code owner May 5, 2023 21:32
@mumanity mumanity requested review from iwysiu and idastambuk May 5, 2023 21:32
@CLAassistant
Copy link

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.


Emily Dodds seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
Copy link

github-actions bot commented May 5, 2023

Backend code coverage report for PR #190
No changes

@github-actions
Copy link

github-actions bot commented May 5, 2023

Frontend code coverage report for PR #190

Plugin Main PR Difference
src 51.21% 51.75% .54%

@github-actions
Copy link

github-actions bot commented May 5, 2023

Levitate is-compatible report:

🔍 Resolving @grafana/data@latest...
🔍 Resolving @grafana/ui@latest...
🔍 Resolving @grafana/runtime@latest...
🔍 Resolving @grafana/e2e-selectors@latest...

🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.5.2...
✔ Found @grafana/data version 8.4.11 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.5.2...
✔ Found @grafana/ui version 8.4.11 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.5.2...
✔ Found @grafana/runtime version 8.4.11 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.5.2...
✔ Found @grafana/e2e-selectors version 8.4.11 locally

✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors

@idastambuk
Copy link
Contributor

Hi @mumanity, thanks for submitting this!
I looked at the screen recording and I have a question to help me understand the motivation behind the change:

What is the user experience we're trying to improve? Is it
a. not seeing the "Request video" section when the panel is smaller? However, I notice at some point when resizing the panel, the buttons cover the video controls, which isn't ideal.
b. Something else I'm not seeing

It's not a particularly elegant resizing experience to begin with, so there's certainly room for improvement. :)

An additional thing - When I build the plugin, I'm not having the css applied, probably because the panel-content class doesn't exist anymore as such.
I would suggest, rather than adding an .scss file and in order to stay consistent, to use the css() method here to apply the styles targeting just the id inside the wrapper.

@mumanity
Copy link
Collaborator Author

mumanity commented May 8, 2023

Hi idastambuk,

The request came from another member of our team to improve the UX by making sure the controls were readily available no matter the video container size. The previous implementation didn't allow viewers to see the video-upload-request-ui without having to resize the panel. This change helps ensure easy access to the component at any given size.

@idastambuk
Copy link
Contributor

Hi idastambuk,

The request came from another member of our team to improve the UX by making sure the controls were readily available no matter the video container size. The previous implementation didn't allow viewers to see the video-upload-request-ui without having to resize the panel. This change helps ensure easy access to the component at any given size.

Hi @mumanity thanks for the clarification. In that case, you can go ahead and apply the suggested correction and sign the CLA, so we can proceed with the merge

@mumanity
Copy link
Collaborator Author

Hi idastambuk,
The request came from another member of our team to improve the UX by making sure the controls were readily available no matter the video container size. The previous implementation didn't allow viewers to see the video-upload-request-ui without having to resize the panel. This change helps ensure easy access to the component at any given size.

Hi @mumanity thanks for the clarification. In that case, you can go ahead and apply the suggested correction and sign the CLA, so we can proceed with the merge

Will do! Thanks for the feedback. :)

@mukeshsahay mukeshsahay merged commit 31330ee into main May 23, 2023
4 of 5 checks passed
@mukeshsahay mukeshsahay deleted the videoControls branch May 23, 2023 22:31
@iwysiu
Copy link
Contributor

iwysiu commented May 24, 2023

Hi! It's fine that this was merged, but can @mumanity sign the CLA?

@mumanity
Copy link
Collaborator Author

Hi! It's fine that this was merged, but can @mumanity sign the CLA?

I clicked on the details link yesterday & went through the process. Each time I went back it said I had already signed the CLA. Is there another link that I can reference to confirm? I thought it was odd that after signing it wasn't updating.

@iwysiu
Copy link
Contributor

iwysiu commented May 24, 2023

Hmm, once in a while the CLA assistant has problems and gets stuck, I'll take a look. Thanks for telling me!

@iwysiu
Copy link
Contributor

iwysiu commented May 24, 2023

I reran it and it didn't pass, but I realized that this commit has a coauthor. Did Emily sign the CLA?

@mumanity
Copy link
Collaborator Author

Yes. It looked like it was letting me sign & then not showing approved due to multiple emails setup in my github. We fixed it & shouldn't see any more CLA stuff for my account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants