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

Adding test to timed text editor #193

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

emettely
Copy link
Contributor

Is your Pull Request request related to another issue in this repository ?

Not from this issue, but DPE has a dependency on react-transcript-editor and we want to start adding tests to it.
bbc/digital-paper-edit-electron#4

Describe what the PR does
Adding the most basic test to timed-text-editor and also removing CustomEditor because it's a wrapper component for one single function - we don't really need it.

State whether the PR is ready for review or whether it needs extra work
Ready

Additional context

@pietrop
Copy link
Contributor

pietrop commented Sep 17, 2019

removing CustomEditor

You might want to check this change against this issue #159 (which is the reason why it was initially separated into its own component)

see #160 for more details.

TL;DR: using react inspector tool highlight updates you can see if removing CustomEditor as separate component increases the number of unnecessary re-renders. You'll have to judge it based on colour.

@emettely
Copy link
Contributor Author

emettely commented Sep 17, 2019

A very good catch, the re-renders definitely don't happen for the original master.
Screen Shot 2019-09-17 at 16 12 52

vs this branch:

Screen Shot 2019-09-17 at 16 14 07

onChange and handleKeyCommand makes the Editor re-render.

@emettely
Copy link
Contributor Author

emettely commented Sep 17, 2019

facebook/react#16437 (comment) The original highlighting function has disappeared but instead you can use the profiler to record the rendering.

@emettely
Copy link
Contributor Author

Added back the Editor as a memoized react component, and now the re-renders are gone.
Screen Shot 2019-09-17 at 16 48 32

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.

2 participants