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: Add Embed #61 #72

Merged
merged 28 commits into from
Nov 23, 2023
Merged

feat: Add Embed #61 #72

merged 28 commits into from
Nov 23, 2023

Conversation

Dhanus3133
Copy link
Contributor

Added Embed Feature

image

Solves #61
/claim #61

<div class="relative">
<%= label(@form, @key, @name, class: "block text-sm font-medium #{@labelClass}") %>
<div class="mt-1">
<%= text_input(@form, @key,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alxlion I made it as a text_input instead of textarea here. When I try textarea the user input value are not been updated not sure what the issue was though.

image

@alxlion
Copy link
Contributor

alxlion commented Oct 31, 2023

I'll check that asap this week, thanks !

@joleaf
Copy link
Contributor

joleaf commented Nov 1, 2023

I think, that there should be a way to set the current embed inactive. Because, if it is activated, and you want to show the slide again, currently there is no way to do it.

So, there should be a way to deactivate the active embed, e.g. by clicking on "Active"

@Dhanus3133
Copy link
Contributor Author

@joleaf thanks for your feedback. I've now added that feature to toggle the status for everything (form, poll, embed)

@alxlion
Copy link
Contributor

alxlion commented Nov 3, 2023

I've encountered several bugs:

1 - No more import button on interactions popup

CleanShot 2023-11-02 at 19 52 30@2x

2 - When you enable a poll and then an embed, the embed doesn't show (presenter + attendee view).

In the specs, I said "When an embed is enabled and is in the current slide position, show it in full screen.". Currently, the embed is not in full screen. It needs to be in full screen like the slide without hiding attendees messages.

I didn't define what to show on the attendee view, I noticed that you display the current embed. This could be good but only if the user select "Attendee can access the embed on the device" when creating an embed.

Good job for what you did 👍

@Dhanus3133
Copy link
Contributor Author

@alxlion I am not sure why it doesn't work for you. I've checked(again) on both the points you have mentioned, seems to be working fine for me but displaying it in full screen is still an issue, I will also look into the changes you told too. Can you check it again? Thanks

@Dhanus3133
Copy link
Contributor Author

I didn't define what to show on the attendee view, I noticed that you display the current embed. This could be good but only if the user select "Attendee can access the embed on the device" when creating an embed.

Just to confirm are you mentioning to add "Enable Embed" in the Attendee settings something like this
image

@alxlion
Copy link
Contributor

alxlion commented Nov 3, 2023

I didn't define what to show on the attendee view, I noticed that you display the current embed. This could be good but only if the user select "Attendee can access the embed on the device" when creating an embed.

Just to confirm are you mentioning to add "Enable Embed" in the Attendee settings something like this image

I thought this would happen during embed creation, with a checkbox in the form.

I am not sure why it doesn't work for you. I've checked(again) on both the points you have mentioned, seems to be working fine for me but displaying it in full screen is still an issue, I will also look into the changes you told too. Can you check it again? Thanks

I'll check that asap

@alxlion
Copy link
Contributor

alxlion commented Nov 8, 2023

Hey @Dhanus3133 , i checked your last commits, it works well ! The last step remaining is showing the embed in fullscreen 😃

@Dhanus3133
Copy link
Contributor Author

Great! I have also found an issue with the shortcut key, using the phx-key causes a clash and toggle happens also with the input fields in the popup window. I have tried disabling the event when it is in the input field but failed, will again try that and there is any other way to avoid this problem? And on the fullscreen should that be only for the presenter right without hiding messages?

@alxlion
Copy link
Contributor

alxlion commented Nov 10, 2023

Great! I have also found an issue with the shortcut key, using the phx-key causes a clash and toggle happens also with the input fields in the popup window. I have tried disabling the event when it is in the input field but failed, will again try that and there is any other way to avoid this problem?

It is because of the ClaperWeb.Component.Input.check in manage.html.heex, to fix it you have to replace shortcut by a condition like:

shortcut={if @create == nil, do: "Q", else: nil}

And on the fullscreen should that be only for the presenter right without hiding messages?

Yes full-screen only on the presenter mode

@Dhanus3133
Copy link
Contributor Author

Hi @alxlion , I've addressed the changes you mentioned. Could you please review when you have time? Thanks!

@alxlion
Copy link
Contributor

alxlion commented Nov 15, 2023

Nice work 🥳 ! I made some change request just to change "embed" to "web content" in the views, "embed" could be too technical for some users.
And add a validation rule to allow only iframe in the content.

I will merge and validate the PR after that 😉

@joleaf
Copy link
Contributor

joleaf commented Nov 15, 2023

Is it possible to paste only the youtube link? Because i think some users may not know how to create an iframe..

@alxlion
Copy link
Contributor

alxlion commented Nov 16, 2023

Is it possible to paste only the youtube link? Because i think some users may not know how to create an iframe..

Using an iframe may present a technical challenge for some users, yet it offers greater versatility in terms of the content that can be embedded. For instance, embedding a Google Slide directly using its URL is not possible. Instead, it requires additional steps: navigate to 'File', 'Share' and select 'Publish to web', and then copy the provided iframe code.

I favor the approach of supplying comprehensive documentation that guides users through the process of embedding various types of content, such as YouTube, Vimeo, or Google Slides. This is preferable to limiting the ability to embed content to a few services that can be embedded only with a simple URL.

@Dhanus3133
Copy link
Contributor Author

And add a validation rule to allow only iframe in the content.

Committed the changes for validating the embed.

@alxlion alxlion merged commit d398906 into ClaperCo:dev Nov 23, 2023
1 check passed
@alxlion
Copy link
Contributor

alxlion commented Nov 23, 2023

@Dhanus3133 Well played! Your PR has been merged 😉

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

Successfully merging this pull request may close these issues.

4 participants