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

Speakername is editable even when edit function is disabled #133

Closed
murezzda opened this issue Apr 8, 2019 · 3 comments · Fixed by #207
Closed

Speakername is editable even when edit function is disabled #133

murezzda opened this issue Apr 8, 2019 · 3 comments · Fixed by #207
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@murezzda
Copy link
Contributor

murezzda commented Apr 8, 2019

Describe the bug
Setting isEditable to false does not prevent editing of speaker names.

To Reproduce
Steps to reproduce the behavior:

  1. Set isEditable in the TranscriptEditor component to false.
  2. Click on speaker label to edit the speaker.

Expected behavior
Speakers are also disabled from edits if isEditable is false

Additional context
If this change is implemented, maybe remove the edit-icon from the speaker.
logo

@murezzda murezzda added the bug Something isn't working label Apr 8, 2019
@pietrop
Copy link
Contributor

pietrop commented Apr 8, 2019

Good catch, as you might remember from PR #103 in

Leaving some notes on possible things to look at to address this. it might be a matter of

and to remove the icon if not editable (good call)

  • In WrapperBlock speakerElement would need the isEditable param so that in the SpeakerLabel component it could know when to hide the speaker icon.

@pietrop pietrop added help wanted Extra attention is needed good first issue Good for newcomers labels Apr 8, 2019
@pietrop
Copy link
Contributor

pietrop commented May 13, 2019

Hi @RizwanSyed357
Thanks for reaching out and welcome to the project!

The text is editable checkbox, is where you can toggle the isEditable attribute for TranscriptEditor in the demo app - (click load demo first)

this prop is being passed to TimedTextEditor in TranscriptEditor - and then in TimedTextEditor state

At the moment to isEditable attribute is implemented at line #L104 of TimedTextEditor under the DraftJS onChange event listener

if it's set to true then changes are saved if false then doesn't update state, making component read only.


This is how it is currently implemented, there might be room for a refactor using draftJs readOnly prop.


re more specifically with this issue of not allowing to edit speakers when isEditable is set to false for TranscriptEditor these are some of the steps from the previous comment worth exploring

let me know if you have any further questions or any of this is not clear, happy to help.

@pietrop
Copy link
Contributor

pietrop commented May 28, 2019

Hi @RizwanSyed357,

I tried readOnly that didn't seem to work, unless I am missing something from the draftJs docs.

But yes, feel free to raise a PR happy to have a look, and see if we can figure this out, I suspect there's something to do with propagating changes from TimedTextEditor to WrapperBlock
and the communication between those two components.

There's another PR where we have tried to address some of the inter component communication to reduce unnecessary re-renders #160 which is still pending review, but might help.

pietrop pushed a commit to pietrop/react-transcript-editor that referenced this issue Oct 7, 2019
emettely pushed a commit that referenced this issue Oct 9, 2019
* fixes issue #133

* Working

if isEditable is true can edit speaker label, if false, cannot

* adjusted CSS accordingly
emettely added a commit that referenced this issue Oct 10, 2019
* fixes issue #133

* Working

if isEditable is true can edit speaker label, if false, cannot

* adjusted CSS accordingly

* moved CustomEditor in separate file

* speaker prompt keeps current speaker name

* brought in changes from previous PRs

master...philmcmahon:update-all-speakers#diff-cdf6a9957d22ff2efacaaf5fb2876a8aR58 it works but does not seem to rerender after change

* names change but component doesn't re-render

* changes don't propagate straight away but they do after you edit text

* cleaned up still no auto re-rendering

* works

added shoudl component update and component did mount in wrapper block to update speaker label

* changing spekaer triggers auto save callback

* Removed space

* Update packages/components/timed-text-editor/CustomEditor.js

Co-Authored-By: Eimi Okuno <[email protected]>

* Update packages/components/timed-text-editor/WrapperBlock.js

Co-Authored-By: Eimi Okuno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants