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

[WIP] Add Patch-Package, Patch nested library, update index #5

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

CrystalJewell
Copy link

  • Patch-Package has been added.
    • There is a nested dependency that needs functionality changed to make it work reliably. This package will allow us to fix that functionality without having to also fork that dependency.
  • @liveview/customer-sdk package has been patch to fix an auth bug it has.
  • index.js has been updated to

@CrystalJewell CrystalJewell self-assigned this Apr 2, 2021
@@ -377,9 +377,6 @@ export default class LiveChat extends Component {
}

shouldDisableComposer = () => {
if (!this.state.onlineStatus && !this.state.chatActive) {
return true
}
if (this.state.queued) {
Copy link
Author

Choose a reason for hiding this comment

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

@grossvogel @ryanalexanderbrashear What do you all think about instead of removing this completely, we create a new prop that would allow for this to be turned off and on?

Something like

if (!this.state.onlineStatus && !this.state.chatActive && !this.state.allowOfflineMessages) {
			return true
		}

Choose a reason for hiding this comment

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

Yeah that'd be a bit more friendly, like what you have except I think it'd be !this.props.allowOfflineMessages (instead of ...state...)

@CrystalJewell
Copy link
Author

@ryanalexanderbrashear @grossvogel If I were tests, where would I live in this package? Also is it worth spending the time to write tests for this? I'mma say yes, because it doesn't have any and we're having to do all this to get it to work properly but also 🤷🏻‍♀️ If I do write some, it will be in a different PR so we don't hold up getting these changes in

@grossvogel
Copy link

@CrystalJewell Test framework setup for react native isn't super straight-forward (and/or I'm just not good at it), and I'm not really sunny on the value of this package as a long-term investment... so I personally wouldn't spend too much time on that but tests are always good

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