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

video insertion now works on the Add Notes screen #188

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AmarHadzic
Copy link
Collaborator

Fixes #186

What was changed
The result of this PR is that it allows users to insert videos into their notes on the Add Notes screen. This results in a positive and enjoyable user experience.

Why was it changed
Currently, the "Add Notes" page does not allow users to attach videos to their notes, causing a poor user experience. With this change, users will be able to attach videos to notes, giving the user more flexibility and usability of the notes feature. Important to note that the Rich Text Editor was migrated to 10Tap. So this change was necessary for the videos to be able to be attached to notes.

How was it changed
In the addVideoToEditor, editor.setImage() function was used to insert a video into the notes. editor.injectCSS() was used to inject css styling to the video to make it 100px by 100px.

UI after changes
image

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@@ -63,6 +63,14 @@ describe('AddNoteScreen', () => {
expect(getByTestId('RichEditor')).toBeTruthy();
});

it('renders the save button', () => {
const routeMock = { params: { untitledNumber: 1 } };
const { getByTestId } = render(<AddNoteScreen route={routeMock as any} />);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great job leaving a test

// Append the video to the existing content
editor.setImage(videoUri);

editor.injectCSS(`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is helpful!


editor.injectCSS(`
video {
width: 100px !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we have to change the height and width? and Should this be in a styles thing?

@rcAsironman
Copy link
Collaborator

@AmarHadzic You are halfway there. I found some issues with your code, and I will provide screenshots of each issue below.

issue 1:
The video in the editor doesn't have controls (play, pause, etc.).

image

issue 2:
The video is not being rendered on android

Your output on Android:

image

Expected output:

The video shouldn't occupy 100% of the editor's width.
image

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.

Implement Video Insertion Functionality in Text Editor with Block-Level Display
3 participants