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

Feature/new oauth server name #108

Merged
merged 6 commits into from
Oct 24, 2017
Merged

Conversation

ivanzusko
Copy link
Contributor

No description provided.

@ivanzusko ivanzusko requested a review from rafaeljesus October 23, 2017 13:04
@@ -61,6 +176,29 @@ const oAuthServerSchema = {
preserve_host: false,
listen_path: '',
upstream_url: '',
upstreams: {
Copy link
Contributor

Choose a reason for hiding this comment

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

If all upstreams schemas has the same structure we could declare one and reuse over the code, so we don't repeat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, but here is one but:

in future, this schema, probably, will come from the server side...

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha!

@@ -31,6 +31,8 @@ class CorsPlugin extends PureComponent {
render() {
const { apiSchema, className, edit, name, handlePluginExclude, plugin, pluginName } = this.props;
const b = block(className);
console.error('nem_____', name);
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad 🥊

this.props.saveOAuthServer(this.props.location.pathname, values);
const transformedValues = transformFormValues(values, true);

this.props.saveOAuthServer(this.props.location.pathname, transformedValues);
Copy link
Contributor

Choose a reason for hiding this comment

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

function composition would be possible here too, if parameter order does not fit we could reorder it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it would be a bit harder then it looks like at first. I'll investigate and make separate PR related to this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue: #110

const createOptions = (list1, list2) => {
const combinedListOfUnitsAndLabels = R.zip(list1, list2);

return combinedListOfUnitsAndLabels.map(item => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

const asLabelValue = item => ({ label: item[1], value: item[0] })
return R.zip(list1, list2).map(asLabelValue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again my bad... this code is redundant here... 🔫

const createOptions = (list1, list2) => {
const combinedListOfUnitsAndLabels = R.zip(list1, list2);

return combinedListOfUnitsAndLabels.map(item => ({
Copy link
Contributor

@rafaeljesus rafaeljesus Oct 23, 2017

Choose a reason for hiding this comment

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

same as line 96

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the same 😞

@rafaeljesus
Copy link
Contributor

👍

@ivanzusko ivanzusko merged commit 6a48def into master Oct 24, 2017
@ivanzusko ivanzusko deleted the feature/new-oauth-server-name branch October 24, 2017 12:15
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