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

Added parameter for hiding save button, straight property pass-through #177

Closed
wants to merge 1 commit into from

Conversation

murezzda
Copy link
Contributor

@murezzda murezzda commented Aug 6, 2019

Is your Pull Request request related to another issue in this repository ?
This PR references #176.

Describe what the PR does
The TranscriptEditor tag has a new parameter showSaveButton. This is a boolean which is passed down to PlayerControls, where it hides or shows the save button.

State whether the PR is ready for review or whether it needs extra work
This is a example PR for this Problem which is very simple. If there is interest of developing such a feature to a greater extend, it would be better to use a more structured approach like a configuration file instead of passing down multiple parameters for each individual component.

@pietrop
Copy link
Contributor

pietrop commented Aug 13, 2019

still need to discuss this with @jamesdools
But a few thoughts for now,

  • Could this be modified to have a default as true?
  • Removing the save button doesn't stop the auto save in local storage from doing it's thing. Is this going to be a problem?

@jamesdools
Copy link
Contributor

Agreed, @pietrop - I think it's a question on whether we do this (toggling the rendering on/off) or being able to map the save function to a customisable thing.

Also yes, if it is not rendering but still autosaving, will that cause problems when it tries to read from local storage as you boot it up.

Sidenote: as per the PR on optimising re-renders (#175), trying to move creating new elements out of the render block.

Will pick this up soon - we have a similar problem on multiple save buttons when integrating this with larger applications.

@pietrop
Copy link
Contributor

pietrop commented Oct 9, 2019

@murezzda see if this PR (that has been merged) addresses your use case as well #200

@murezzda
Copy link
Contributor Author

murezzda commented Oct 9, 2019

@pietro yes, #200 seems to address this as well. Thanks a lot!

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.

3 participants