Create Dialog component

[chalettu]

Created Dialog component that allows for editing and viewing of dynamic dialogs

[chalettu]

Demo of dialog component

[chalettu]

@eclarizio . Here is the new dialog component in all its glory. Please take a look and provide feedback. Thanks in advance.

[eclarizio]

If I understand this correctly, this would change the way cascading currently works, right? This is going to loop through all the fields to refresh and immediately fire off the .refreshField function. If, in the list of fieldsToRefresh, the 2nd one depends on the 1st one completing, this will not behave like it does currently.

I think instead of iterating, we need to do something recursive like:

public updateRefreshableFields(listOfFieldsToExclude) {
  const fieldsToRefresh = _.without(this.refreshableFields, listOfFieldsToExclude);
  return if fieldsToRefresh[0] === undefined;

  const firstFieldToRefresh = fieldsToRefresh[0];
  const listOfFieldsToExclude = listOfFieldsToExclude.concat(firstFieldToRefresh);

  firstFieldToRefresh.refreshField({ field:this.dialogFields[firstFieldToRefresh] }).then(data) => {
    this.dialogFields[firstFieldToRefresh] = this.updateDialogFieldData(firstFieldToRefresh, data);
    this.updateRefreshableFields(listOfFieldsToExclude);
  });
}

[eclarizio]

@chalettu Thanks! Looks good so far, just noticed one thing regarding the potential change I addressed above, will continue looking for more things but again on first glance looks great. Super excited to be able to use this 👍

[AllenBW]

Noted a few glaring items, will continue to review more thoroughly in the coming days 🙇‍♀️

[AllenBW]

looks like the dist didn't build correctly, still has comments and is expanded

[AllenBW]

Eeeeeek same here, dist definitely didn't build correctly, js isn't concatenated or uglified

[AllenBW]

not sure if the spacing between the ngInject matters here 🤔

[AllenBW]

For sure something is wack with dist build, new content isn't concatenated or uglified :-/ :thinking:

[himdel]

Keep to the project's standards please. 2 spaces, not 4.

[himdel]

@chalettu Still relevant ;)

[himdel]

Missing space ;)

[himdel]

Same here, bad spacing.

[chriskacerguis]

@himdel good catch. Maybe we should add linting check to travis?

[himdel]

And here :)

[himdel]

Still relevant.

[himdel]

And here..

[himdel]

@chriskacerguis That makes sense :) maybe not so much travis as code climate, but sure :)

[himdel]

And just a heads up, after we merge #83, you can drop the dist/ changes from here, dist will get updated automatically when travis succeeds on master.

[chriskacerguis]

@chalettu Could you please rebase and make the changes people requested, so we can merge this?

[chriskacerguis]

@chalettu Could you please rebase and make the changes people requested, so we can merge this?

[chalettu]

In the process of rebasing and going over the linting related issues.

[chalettu]

@himdel , I went through and found where the linter that is in place had messages about formatting. I addressed all linting related warnings and also reformatted the HTML templates to line up more as expected.

[himdel]

This whole function could be

let values = _.sortBy(data.values, data.options.sort_by === 'value' ? 0 : 1);
return data.sort_order === 'ascending' ? values : values.reverse();

(not saying it has to be this short, just that you probably don't need to reimplement sortBy by yourself)

[chalettu]

Good feedback. Some of these functions are carryovers from the existing components, this is a solid enhancement

[himdel]

This whole function is ! value

[chalettu]

Ended up removing it. Realized I must have changed something to where it wasn't needed.

[himdel]

missing gettext?

[himdel]

missing gettext?

[himdel]

The data comes that way from the server, right? :(

I don't like all those [0] and [1] instead of .value, .description, and that combined with taking the first element of array means this code is completely opaque until you know what the data looks like.

(I guess this is mostly a rant, unless you can change how the data comes from the server, right? :))

[chalettu]

Yeah, on this one it is how the data comes back from the server. It annoys me as well. I might tweak it to make it more readable.

[himdel]

stray ngInject?

[himdel]

You need to remove this file from git, and rebase so that you get the .gitiignore entry for it ;).

We can't merge with this here..

[himdel]

Same goes for dist/js/ui-components.js.map

[chalettu]

Good catch, i had rebased and not sure how it got back in. Will remove it.

[himdel]

Still bad spacing

[himdel]

And here, 2 spaces not 4.

[chalettu]

This made it past whatever linting rules are in place in the repo. I can switch this to two spaces though.

[himdel]

Would be nice to get some newlines here, at least between functions, and between classes.

[chriskacerguis]

@serenamarie125 we are componentizing the dialog rendering system so we can use it both in the SUI and ops ui (so we don't need to maintain it in two places)

[martinpovolny]

@serenamarie125 : In ManageIQ we can create custom dialogs. These are mostly used to order services. This is a component "plays" such custom dialog for a user. It let's the user fill in stuff and returns the values. Typically the values would be passed to automate to execute some automation.

There's another implementation of this in the OPS UI and the plan is to share a single (Angular) implementation of the dialog player.

[himdel]

@martinpovolny Looks like @manageiq/ui-components is right now, but .. ☝️ July 24, 2017 5:16 PM .. looks like by not having dist/ in git, it's also excluded from the npm package.. (looking into it..)
.. EDIT: fixed in @manageiq/ui-components 0.0.17

Otherwise, looks good :).

[chalettu]

@martinpovolny Our implementation strategy was going to be to request another version bump to NPM and we would update the SUI package.json to reflect it.

[serenamarie125]

LGTM, i will create some issues regarding UX review items regarding code which existed in the SUI.

[serenamarie125]

LGTM, i will enter GH Issues around some of the concerns i have with existing code.

[chriskacerguis]

@martinpovolny @himdel @eclarizio any other issues to address, or could we merge?

[chalettu]

@serenamarie125 , I moved the error messaging per your request. Thank you for your feedback.

[chalettu]

@chriskacerguis , I rebased and addressed the last piece of feedback received.

[chriskacerguis]

BZ - https://bugzilla.redhat.com/show_bug.cgi?id=1502310