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

Implement Edit Data Source #29

Merged
merged 14 commits into from
Jul 13, 2018
Merged

Conversation

josemigallas
Copy link
Contributor

JIRA: https://issues.jboss.org/browse/AEROGEAR-3544

This PR introduces big changes to the DataSourcesDialog:

  • AddDataSourceDialog has been repurposed into an "abstract" BaseDataSourceDialog.
  • AddDataSourceDialog and EditDataSourceDialog are new children classes that shares common validation logic.
  • Each Component implement a different query (create and update).
  • BaseDataSourceDialog has "abstract" methods that children classes must implement in order to correctly setup things like title, disabled forms, the submit button.

Important changes in DataSourcesContainer:

  • A new state property has been created selectedDataSource that is updated whenever the "Edit" option under the three dots dropdown is selected. That property is then passed into the appropriate modal as a prop. This approach could (or would) be used to "Delete Data Source" as well. Pinging @cfoskin .

default:
return null;
}
isDisabled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be a constant instead of a function if all it does is return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TL;DR: NO 😄
This is an implementation of super.isDisabled().
What it is for? It determines which FormControls are disabled. For example when adding, you want all of them available (maybe, for now) but in case of editing, you want to disable "Data Source Type". If you take a look at BaseDataSourceDialog, all the forms have this property disabled={this.isDisabled(<control-id>)}.
Example: Let's say while creating a Postgres data source, you want to enforce timestamp data, then you could do this by adding the condition inside this method:

isDisabled(controlId) {
    if (controlId === "timestampData" && this.state.type === DataSourceType.Postgres) {
        return true;
    }
    return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks for the explanation ;-)

pb82
pb82 previously approved these changes Jul 12, 2018
Copy link
Contributor

@pb82 pb82 left a comment

Choose a reason for hiding this comment

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

Verified locally

@psturc
Copy link
Contributor

psturc commented Jul 12, 2018

Verified the functionality, just a little detail about the buttons naming:

  • JIRA description says we should change the "Add" button to "Save"
  • changed name of the button (Edit) is displayed also in the form when I'm adding a new data source

screen shot 2018-07-12 at 10 32 53

Copy link
Contributor

@darahayes darahayes left a comment

Choose a reason for hiding this comment

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

Great work @josemigallas!

darahayes
darahayes previously approved these changes Jul 12, 2018
validations: {
name: null,
type: "success",
inMemoryValues: "success"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darahayes I think I understand what you really meant in last PR with this field. At that time I didn't realize these "values" really belonged to config.options. I think it will work with a single generic options that contains all the different possibilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry wrong line, I meant line 28

@josemigallas
Copy link
Contributor Author

@darahayes @psturc @pb82
Thank you so much for the reviews.. if only you could do a quick review it again 😅

  • Fixed dialog button name
  • DataSource full editable (name and options/config)
  • Some minor refactorizations

@josemigallas josemigallas dismissed stale reviews from pb82 and darahayes July 12, 2018 15:47

New changes

Copy link
Contributor

@pb82 pb82 left a comment

Choose a reason for hiding this comment

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

Verified latest changes

@pb82 pb82 merged commit 3a1f23a into aerogear-attic:master Jul 13, 2018
@josemigallas josemigallas deleted the AEROGEAR-3544 branch August 27, 2018 07:27
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