Skip to content

[ML] Converts the custom URL editor to EUI / React#21094

Merged
peteharverson merged 1 commit intoelastic:masterfrom
peteharverson:ml-custom-url-editor-react
Jul 24, 2018
Merged

[ML] Converts the custom URL editor to EUI / React#21094
peteharverson merged 1 commit intoelastic:masterfrom
peteharverson:ml-custom-url-editor-react

Conversation

@peteharverson
Copy link
Contributor

Converts the custom URL editor, as used in the flyout when editing an ML job, from Angular to EUI / React.

Current Angular based component:
custom_url_angular

New EUI / React component:
custom_url_react

A couple of enhancements are planned for future PRs (expect 6.5) when editing existing custom URLs, to auto expand the URL value field from a text field to a text area on focus, plus extra validation for the fields to disable the edit job flyout save button if for example the URL label or value are blank.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui



export function getNewCustomUrlDefaults(job, dashboards, indexPatterns) {
// Returns the settings object in the format used by the custom URL editor
Copy link
Member

Choose a reason for hiding this comment

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

nit, we normally put the descriptive comment above the function

}];
}

function onLabelChange(e, customUrl, setEditCustomUrl) {
Copy link
Member

@jgowdyelastic jgowdyelastic Jul 23, 2018

Choose a reason for hiding this comment

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

I think these functions would be better inside CustomUrlEditor and it converted to a class.
Only because you're having to pass setEditCustomUrl in to store the data.
i know we discussed this but i didn't realise the plan was to pass in the function which is called to do the storing the data.
but i'm happy to go with the majority on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good candidate for testing out the React Context API, to avoid having to pass down customUrl and setEditCustomUrl several levels through Props. Will plan to check that out in a later PR.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants