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

Renaming and restructuring directives and components based on ng naming guidelines #541

Merged
merged 29 commits into from
Feb 8, 2018

Conversation

ddincheva
Copy link
Contributor

Closes #536

Additional information related to this pull request:

@ddincheva ddincheva requested a review from zdrawku February 1, 2018 12:27
@zdrawku zdrawku requested a review from kdinev February 1, 2018 13:14
@@ -11,7 +11,7 @@ import {
@Directive({
selector: "[igxInput]"
})
export class IgxInputClass implements DoCheck {
export class IgxInputDirective implements DoCheck {
Copy link
Member

Choose a reason for hiding this comment

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

This would again be inconsistent, as the IgxLabel is not called IgxLabelDirective. Please check all the directives, and if they all follow the Igx<DirName> format, then leave it as IgxInput. If not, then we should discuss which naming to go with.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I checked. The whole thing is totally inconsistent. The IgxFilterDirvective, IgxRippleDirective, IgxDraggableDirective and IgxDroppableDirective are all located under the directives folder and follow the Igx<DirName>Directive naming.

The IgxLabel, IgxInputClass, IgxLayout and IgxButton directives are all in their own folders and follow mostly the Igx<DirName> naming, except for the input.

I think we should unify all this and either move them all under directives and call the classes uniformly, or move them all into their all folders and again name them uniformly, or move them all into their own folders under the directives folder and again name them all uniformly.

Copy link
Contributor

@zdrawku zdrawku left a comment

Choose a reason for hiding this comment

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

We are going to make consistent all directives and modules first

@zdrawku
Copy link
Contributor

zdrawku commented Feb 2, 2018

Issue description is updated based on the latest comments and recommendations

@kdinev
Copy link
Member

kdinev commented Feb 5, 2018

Since this is a major breaking change that affects pretty much every component and directive, this should come with all necessary documentation updates, as well as sample updates ready to be deployed at the time of tagging. Please implement all README updates, demo updates, and docfx updates. Make sure to check with the igniteui-cli team for all view and project template updates.

Another thing is that checks for the correct naming should be performed on a CI level. Please implement this as well!

@zdrawku zdrawku changed the title renamed igxInput Renaming and restructuring directives and components based on ng naming guidelines Feb 5, 2018
@zdrawku zdrawku requested a review from damyanpetev February 5, 2018 08:30
@zdrawku
Copy link
Contributor

zdrawku commented Feb 5, 2018

@ddincheva I have updated the issue with additional note, all documents (topics, readme.md files and wiki pages) should be updated with the structural and naming changes (demos, docfx). Also, could you move dragdrop, filter and ripple directives in separate folders within 'directives' folder as well.

@damyanpetev we are renaming some directives and components, this would affect the ig cli project. Is it possible someone from your team to check what additional changes should be implemented from cli point of view, and use this pull request as a base. This is important as it would introduce some breaking changes.

@kdinev kdinev mentioned this pull request Feb 5, 2018
@zdrawku
Copy link
Contributor

zdrawku commented Feb 6, 2018

The only task that left is to add codelyzer to the repo. @ddincheva I am going to add this to the requirements in the issue.

@zdrawku
Copy link
Contributor

zdrawku commented Feb 6, 2018

Related pull request IgniteUI/igniteui-angular-samples#45

@zdrawku
Copy link
Contributor

zdrawku commented Feb 7, 2018

There is conflicting file @damyanpetev could you please have a look

@kdinev kdinev merged commit 530f2c8 into IgniteUI:master Feb 8, 2018
@zdrawku zdrawku mentioned this pull request Feb 9, 2018
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.

4 participants