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

Resolver Detail Hooks #90

Merged
merged 23 commits into from Aug 10, 2018

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Aug 6, 2018

NOTE: built on top of #80, do not merge until #80 it is merged.

This PR adds:

TODO:

@josemigallas josemigallas changed the title [Do Not Merge] Resolver Detail Hooks Resolver Detail Hooks Aug 7, 2018
@psturc
Copy link
Contributor

psturc commented Aug 7, 2018

👀

onPreHookChange(preHook) {
const preHookValidation = Validate([
Validators.String.minLength(1), preHook,
Validators.String.maxLength(255), preHook,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be too small for a URL. The limit seems to be closer to 2000 chars: https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers

@josemigallas josemigallas changed the title Resolver Detail Hooks [WIP] Resolver Detail Hooks Aug 7, 2018
@psturc
Copy link
Contributor

psturc commented Aug 7, 2018

This is more related to #80, but should we warn user about unsaved changes in Resolvers tab also when the page is being reloaded?

URL: {
valid: u => {
try {
return !!new URL(u);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct ! !?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, it's a quick way to parse any value into a boolean.

@josemigallas josemigallas changed the title [WIP] Resolver Detail Hooks Resolver Detail Hooks Aug 9, 2018
return;
}

fetch(url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be done in the backend. Otherwise you only test whether the user's browser can callthat URL, not if the sync backend (probably hosted on some Openshift cluster) can reach it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fetch also doesn't seem to allow cross origin requests by default which also breaks this.

) {
upsertResolver(
id: $id,
schemaId: $schemaId,
dataSourceId: $dataSourceId,
type: $type,
field: $field,
preHook: $preHook,
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add preHook and postHook to the result of the mutation too. Otherwise they will be emptied after save.

@josemigallas josemigallas merged commit 042be6a into aerogear-attic:master Aug 10, 2018
@josemigallas josemigallas deleted the AEROGEAR-7724-hooks branch August 10, 2018 09:50
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.

None yet

4 participants