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.

Sign up for GitHub

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

Jump to bottom

rav1e_js: New demo website with react #2423

Merged
merged 1 commit into from
Jul 4, 2020
Merged

rav1e_js: New demo website with react #2423

merged 1 commit into from
Jul 4, 2020

Conversation

Urhengulas
Copy link
Contributor

This PR rebuilds the example rav1e_js example website with react.js and makes it a bit more visual.

@coveralls
Copy link
Collaborator

coveralls commented Jun 29, 2020 •
edited

Coverage Status

Coverage remained the same at 81.816% when pulling dda14f6 on Urhengulas:jsapi-react-exp into af76f4f on xiph:master.

@lu-zero
Copy link
Collaborator

lu-zero commented Jun 29, 2020

image

Might be nicer to format the config and maybe add a flush button. With some css it would be a great demo :)

@Urhengulas
Copy link
Contributor Author

I actually like plain HTML for the demo purpose, but I will have a look into this :D

@Urhengulas Urhengulas force-pushed the jsapi-react-exp branch 2 times, most recently from 6ba4d58 to 7110a63 Compare June 30, 2020 09:48
@Urhengulas Urhengulas mentioned this pull request Jun 30, 2020
Closed
3 tasks
@lu-zero
Copy link
Collaborator

lu-zero commented Jun 30, 2020

The receive packet button is grayed out. It should not.

@Urhengulas
Copy link
Contributor Author

The receive packet button is grayed out. It should not.

It should only be grayed out when there are no frames yet to be encoded.

@Urhengulas Urhengulas self-assigned this Jun 30, 2020
@lu-zero
Copy link
Collaborator

lu-zero commented Jun 30, 2020

it does not work this way right now :)

vibhoothi
vibhoothi requested changes Jun 30, 2020
View reviewed changes
Copy link
Collaborator

@vibhoothi vibhoothi left a comment

Choose a reason for hiding this comment

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

Earlier when we try, we can send encode it close it,
Then send again, then encode it,
Now we have to send the frames, flush it, then receive it, which seems to be not the right way
Since after encoding we are flushing,

@Urhengulas Urhengulas requested a review from vibhoothi July 1, 2020 22:32
@Urhengulas Urhengulas force-pushed the jsapi-react-exp branch 2 times, most recently from fd1da65 to 3536e3c Compare July 2, 2020 17:44
vibhoothi
vibhoothi requested changes Jul 2, 2020
View reviewed changes
Copy link
Collaborator

@vibhoothi vibhoothi left a comment

Choose a reason for hiding this comment

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

The set looks more matured, I have few queries/concerns, please check the review comments :)

rav1e_js/www/src/App.tsx Show resolved Hide resolved
rav1e_js/www/src/App.tsx Show resolved Hide resolved
rav1e_js/www/src/App.tsx Show resolved Hide resolved
rav1e_js/www/src/App.tsx Outdated Show resolved Hide resolved
@Urhengulas Urhengulas requested a review from vibhoothi July 3, 2020 16:51
vibhoothi
vibhoothi approved these changes Jul 3, 2020
View reviewed changes
Copy link
Collaborator

@vibhoothi vibhoothi left a comment

Choose a reason for hiding this comment

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

I have tested locally on both Safari and Chrome, seems to work fine,

It is now having proper Status for end-user with basic help.

No issues faced, the only thing in future for cosmetic changes we can do is having a better spacing between things, nevertheless not important.
Also, another minor thing we can debug in future is we have [Violation] 'click' handler took 210ms warning in chrome when we do send and receive multiple long packets which is not big concern to see at this point.

Thanks a lot for the patchset.

Next thing we can do is, setting up a demo website and hosting it.

vibhoothi
vibhoothi reviewed Jul 3, 2020
View reviewed changes
Copy link
Collaborator

@vibhoothi vibhoothi left a comment

Choose a reason for hiding this comment

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

Also, I would prefer cleaning up the commits before landing since there are some changes which are being replaced in further commits.

@Urhengulas
Copy link
Contributor Author

Squashed all the commits into one :)

@Urhengulas
rav1e_js: New demo website with react
dda14f6 
Changes:
* javascript
    * interactive encoding with buttons
    * display encoded packets
    * HelpAndAbout section, which explains
      background and functionality
    * Display pretty-printed EncderConfig
* rust
    * add EncoderConfig.toJSON()
lu-zero
lu-zero approved these changes Jul 4, 2020
View reviewed changes
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

It seems fine to me as well.

@lu-zero lu-zero merged commit 26fd6b0 into xiph:master Jul 4, 2020
@Urhengulas Urhengulas deleted the jsapi-react-exp branch July 5, 2020 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vibhoothi vibhoothi vibhoothi approved these changes

@lu-zero lu-zero lu-zero approved these changes

Assignees

@Urhengulas Urhengulas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Urhengulas @coveralls @lu-zero @vibhoothi