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

Hard overwrite on conflict for query owners (re #283) #327

Merged
merged 2 commits into from
Mar 5, 2018

Conversation

washort
Copy link

@washort washort commented Jan 30, 2018

This fixes #283 though I'd love to see a better way to handle an action in a toastr notification than setting a global function as callback.

@jezdez
Copy link

jezdez commented Feb 28, 2018

@washort So with the looming issue of staying close with upstream in terms of API usage I just looked again at the toastr calls there. There are no uses of HTML in toastr popups and I think that means we should stay away from that as well.

In stead I think we can safely use the window.confirm function instead as it's used in various places in the code.

And since our confirmation query only applies to query owners it's okay UX wise.

In other words, depending on $scope.isQueryOwner I think we should just conditionally call window.confirm or toastr.error.

@jezdez
Copy link

jezdez commented Feb 28, 2018

@washort Do you want me to take a stab at this? (I know you've spent lots of time on this already)

@washort
Copy link
Author

washort commented Feb 28, 2018

Sure, if you don't mind having a look.

@jezdez jezdez force-pushed the hard-overwrite-283 branch from c2cf29e to 6508675 Compare February 28, 2018 22:58
@jezdez
Copy link

jezdez commented Feb 28, 2018

@washort Okay, found an own AlertDialog thing actually, which is what is used in other places already that fits the bill better than window.confirm.

@jezdez jezdez force-pushed the hard-overwrite-283 branch from 6508675 to 5079ca2 Compare February 28, 2018 23:01
@washort
Copy link
Author

washort commented Mar 1, 2018

r+, this is obviously a better solution

@jezdez jezdez merged this pull request into master Mar 5, 2018
@jezdez jezdez deleted the hard-overwrite-283 branch March 5, 2018 16:28
@jezdez
Copy link

jezdez commented Jan 25, 2019

This got merged upstream as well.

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.

Owners of queries should be able to hard overwrite
2 participants