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

Gtl module #24

Merged
merged 5 commits into from Sep 21, 2016
Merged

Gtl module #24

merged 5 commits into from Sep 21, 2016

Conversation

karelhala
Copy link
Contributor

Start of gtl module, this module will contain table, grid and list components.

This initial PR introduces services for these components (fetching data from URL) and adds this module to main module.

*/
private static generateConfig(modelName?: string, activeTree?: string, currId?: string) {
let config = {params: {}};
_.assign(config.params, DataTableService.generateModelConfig(modelName));
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to figure out the priority of libraries to use, when there is overlapping functionality. This statement is totally correct but should one use Object.assign(), _.assign(), [or even angular.merge, angular.extend].

Anyway, we probably need a standard approach to this so that each of of doesn't do it differently.
@himdel @martinpovolny wdyt?

Copy link
Contributor

@himdel himdel Sep 21, 2016

Choose a reason for hiding this comment

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

I'd go in the order of [ES6, lodash, angular, jquery], except for when the angular functions are explicitly needed because we want to skip stuff like $id etc. (And except for when you want to use a lodash feature not found in the es6 version)

Copy link
Contributor

Choose a reason for hiding this comment

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

...but then again, that (es6 over lodash) assumes es6-shim or new browsers. We do that in miq, but not sure how far to take the library thing..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be more benevolent with this and put ES6 and lodash at the same level. But then egain, we are using Typescript and we no longer need to use _.map, _.each but use ES6's equivalents.

But if we agree on using more ES6 things over lodash's I'm ok with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be more benevolent with this and put ES6 and lodash at the same level.

No problem with that :). Either way, I guess there's no reason to change this PR..

@mtho11
Copy link
Contributor

mtho11 commented Sep 16, 2016

@karelhala btw I'm glad the loader.ts has changed to index.ts seems much more standard.

@karelhala
Copy link
Contributor Author

New tests were added, so I think it's good to go. WDYT @himdel @martinpovolny @mtho11

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 89.337% when pulling 94da90b on karelhala:gtl_module into 460dcce on ManageIQ:master.

@himdel himdel added this to the Sprint 47 Ending Oct 3, 2016 milestone Sep 21, 2016
@himdel himdel merged commit 26f4a95 into ManageIQ:master Sep 21, 2016
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.

None yet

4 participants