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

Fix #200 component save #201

Merged
merged 14 commits into from
Oct 4, 2019
Merged

Conversation

pietrop
Copy link
Contributor

@pietrop pietrop commented Oct 3, 2019

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

#200

Describe what the PR does

Option 1 in #200 (comment)

In current master to save to server the logic is already outside of the component using ref.

The suggestion is to revisit that, and move the logic for local storage saving outside of the component as an optional module available at user discretion.

This also leaves to the user(developer) the responsibility for the logic on what to do when local storage maxes out. As well as whether they'd rather use some other form of local storage, eg indexDb.

Adds the following attributes to TranscriptEditor

Attributes Description required type
handleAutoSaveChanges returns content of transcription after a change no Function
autoSaveFormat specify the file format for data returned by handleAutoSaveChanges,falls back on sttJsonType. or draftjs no string
  • fileName is no longer used as key to store and retrieve from local storage, but is still in use when exporting transcript in various formats. and it's still optional.
  • logic in onChange to auto save in TimedTextEditor was trigger a bit too often, as it was not inside the check to see if the content of draftJs had changed (this.state.editorState.getCurrentContent() !== editorState.getCurrentContent()) so moved it to only be triggered if content has been modified (inserted, deleted etc..)
  • updated TranscriptEditor storybook (http://localhost:6006/?path=/story/transcripteditor--default)
  • setState is async so changed the logic to use result of updateTimestampsForEditorState when returning data for autoSave.
  • added an auto save text area for inspection and troubleshooting in demo app in storybook next to the analytics text area
  • moved function to save and retrieve from local storage in local-storage.js under the demo app, consider whether it's work, in a separate PR, showcase how local storage save would work in the demo app etc.. I'd say low priority tho?

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

Ready for review

Additional context

related Issue
#176
PRs
#177
#199

Once this PR is merged it be good to

  • update the the storybook on gh pages
  • update npm

@pietrop pietrop marked this pull request as ready for review October 4, 2019 11:37
Pietro added 4 commits October 4, 2019 12:38
only works with laodDemo, other import from computer etc.. still not addressed
…nscript-editor into fix-200-component-save
Copy link
Contributor

@emettely emettely left a comment

Choose a reason for hiding this comment

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

Looks good - thanks for updating the docs + demos also. If I understood correctly, the AutoSaveChanges is what gives the file extension? Not so sure if the autoSaveFormat is an option that you would want to be able to configure.
Anyways, I have one code suggestion (see below).

demo/app.js Outdated Show resolved Hide resolved
demo/app.js Show resolved Hide resolved
@emettely emettely added the bug Something isn't working label Oct 4, 2019
@emettely emettely merged commit 7144325 into bbc:master Oct 4, 2019
@pietrop pietrop deleted the fix-200-component-save branch April 23, 2020 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants