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

Proposal: follow the Angular Style Guide more closely #6847

Closed
mirkonasato opened this issue Jun 10, 2016 · 22 comments
Closed

Proposal: follow the Angular Style Guide more closely #6847

mirkonasato opened this issue Jun 10, 2016 · 22 comments
Assignees

Comments

@mirkonasato
Copy link

The code for an Ionic v2 project generated with the CLI differs in a few ways from the Angular 2 Style Guide.

Of course it's just a matter of style, but keeping things consistent would make it easier for people who just learnt Angular 2 to learn Ionic 2 as well. So it would be nice for the starter code to follow the Style Guide as much as possible.

For example, the starters currently have files named e.g. home-page.js while Style 02-01 in the guide says

Do follow a pattern that describes the symbol's feature then its type. The recommended pattern is feature.type.ts.

so it would be either home.page.js or home-page.component.js, depending on whether you consider page to be a type or not.

For imports Style 03-05 suggests

Do leave one whitespace character inside of the import statements' curly braces when destructuring.

so e.g. import { Component } rather than import {Component}.

For TypeScript code, Style 03-04 now says

Avoid prefixing private properties and methods with an underscore.

(There is no separate Style Guide for JavaScript yet, but I don't think that applies to *.js files.)

@Ionitron Ionitron added the v2 label Jun 10, 2016
@mlynch
Copy link
Contributor

mlynch commented Jun 10, 2016

I'm not a huge fan of the styleguide file naming conventions, I'd rather organize by directory rather than filename. However, keeping consistency with what people are using/learning in Angular 2 world is also important.

In Ionic, a Component that is a page is more than a component, logically speaking. I think in that case calling it about.component.js doesn't make sense. It's not mean to be embedded as a component. I can see about.page.ts working, but then I prefer pages/about/about.ts. Also we are moving away from page as a naming convention and towards view. Perhaps about.view.ts?

If this was iOS we'd call it AboutViewController and it would be obvious how these classes relate in the grand scheme of our app. Going to .component doesn't give enough information about what this component does.

I'm unsure what is best here.

@adamdbradley
Copy link
Contributor

I think we can keep our structure the same for the time being, but agreed we can update the formatting to follow the styleguide, such as import { Component }. Are there any other ones we need to update?

@mirkonasato
Copy link
Author

Imports and no underscore prefix on private properties (typescript only) are the only ones about formatting as far as I can see.

The guide does recommend a folders-by-feature structure though, as opposed to a folders-by-type one.

@adamdbradley
Copy link
Contributor

We'll keep an eye on the style guide for any syntax differences, thanks.

@larssn
Copy link

larssn commented Jun 2, 2017

I'd like to leave my thoughts on this.

A good project structure is meant to allow the dev to quickly find what he is looking for.

Your current structure is fine for small projects, but becomes messy for larger ones, as it might have a component directory with 40 components in it. The human brain is not built for quickly glancing that many items.

Angular2's "Folder-by-feature" structure makes more sense there, where each folder describes a feature-area. So I might have a SalesComponent that is made of many smaller sub-components, so finding stuff related to a SalesComponent is more intuitive: It's in the /sales folder.

Their file naming convention is also flexible and nothing is set in stone, so if you're unhappy about your page being called SalesComponent you can call it SalesPage and rename your file sales.page.ts, the only thing the file naming convention is meant to convey is feature.type.ts.

So my suggestion is embrace it; delegate the responsibility of having to think of how the style guide should be, going forward, to the Angular team. My impression is that you have enough on your plate as it is. 😄

We've switched to Angular's, in my company, as we're doing a big project atm. Unfortunately it means that we can't use the Ionic CLI's generate functionality, as it messes up the structure.

@WhatsThatItsPat
Copy link

@larssn, in case you missed them, here are a few related issues tucked away in other Ionic repos:

@mlynch
Copy link
Contributor

mlynch commented Jun 5, 2017

Not opposed to doing it for just components. We have our own concept for pages so they don't really fit into the style guide

@jaufgang
Copy link

@mlynch, could you explain what you mean by "We have our own concept for pages so they don't really fit into the style guide?" I don't see how pages aren't just a particular type of component.

As suggested by @PatrickMcD in ionic-team/ionic-app-generators#14, for pages you could use *.page.(ts|html|scss) and even *.modal.(ts|html|scss) instead of *.component.(ts|html|scss), if you want to differentiate between pages and other types of components. That would in fact still conform to style 02-02 of the style guide, which allows for inventing a small number of additional type names, and this seems to fit that well.

Do use conventional type names including .service, .component, .pipe, .module, and .directive. Invent additional type names if you must but take care not to create too many.

This is the convention that my team has adopted for our Ionic projects, and it works well.

Considering that this style guide was long ago officially officially adopted by Angular, it really should be considered best practices for any team to follow it, and any development framework built on top of Angular should really strive to follow this guide as closely as possible.

@mlynch
Copy link
Contributor

mlynch commented Jun 15, 2017

Yea that could work. I'm open for whatever. Something we'll need to keep in mind is how we scale this between framework as we add support for react and vue, etc.

Maybe we just try to follow the standards for each as closely as possible?

@mariohmol
Copy link

mariohmol commented Aug 3, 2017

Awesome.. was now just refactoring my ionic project to match the pattern for the other modules in angular.

So here is what i had to change manually to match:

Symbols and FileNames: Class name must follow the type as well, ex HeroComponent and not HeroPage (not just the file name hero.component.ts)

Make in modules : that means not follow the page folder style as we have in ionic. Services and components should be together in the feature style rather than a type folder style

  1. a \hero\ folder would have hero.module.ts wrapping all the components and inserting at once in the main app.
  2. create the hero.routing.ts in this same folder referencing the routes for that feature foldde

Core/Shared: Create the \core\ folder with main app and \shared\ with common stuff


The discussion about using Component or Page to me is kind solved. i'm not quite sure if we should leave the Page for all... The thing is.. neither angular or react uses the type .page , so thats why i'm willing to use just components and have the basics as the design suggests : components/directives/services/modules/pipes

@glebmachine
Copy link

Any news guys?

@mariohmol have you managed with refactoring?

@mariohmol
Copy link

Hi!!

Yeah.. did all the refactor and not using pages at all.. have in same solution another app in angular using the exactly same pattern.. so its much easier to maintaing both projects as they have same structure

thanks!

@msevestre
Copy link

@mariohmol Does your approach work with lazy loaded pages?

@glebmachine
Copy link

glebmachine commented Jan 27, 2018

@mariohmol Could you share step-by-step recipe, or demo repo with your working structure?

@mariohmol
Copy link

All right.. took a long time to answer.. sorry ..

I did a refactor of a open source project that i have to demonstrate how this organization would be. Still need some more work to show more code style examples, but we have the basics here of: Project Template for Ionic3 + Angular 5 + Code Style Spec + TSLint

@mlynch
Copy link
Contributor

mlynch commented Feb 27, 2018

Quick update on this: we are moving to use the Angular CLI and style guide in the next release of @ionic/angular (4.x+). Stay tuned.

@glebmachine
Copy link

glebmachine commented Feb 27, 2018

@mariohmol Awesome, will take a look soon)

@mlynch, how long it will take?
That the main reason why our team not to consider to use ionic.
I'm sure, that lots of people prefer phonegap to ionic, because of yours selfish code style guides.

Why you are not to support canonical @angular/router?
Why you don't support routing modular splitting?
Why you support only sass and build in parallel (drop component styleUrls and style incapsulation)?
And ofcourse, why you still not support @angular/cli, that the hidden gem of angular

That frustrates me, you have done a great job, but fails with angular so much(

@mlynch
Copy link
Contributor

mlynch commented Feb 27, 2018

@glebmachine two reasons: the Angular router and other angular libraries were not built initially to enable native app style functionality. Two: much of Ionic predates Angular standards and tooling.

@mlynch
Copy link
Contributor

mlynch commented Feb 28, 2018

@glebmachine It should be done here soon, meaning weeks not months, coinciding with Ionic v4. We know this is what you all want and we're doing it. We also found a way to make the router work with our navigation, thanks to some help from the Angular team.

@glebmachine
Copy link

glebmachine commented Feb 28, 2018

@mlynch I can’t describe you how excited I am :) this is great news for community, because it will dramatically decrease entry barier for newcoming angular developers

Thank you (not only from me, but from every struggling angular buddies). I hope, You will feel measured statistical results after that!

Feel free to ask any feedbacks or proof-of-concepts

@brandyscarney brandyscarney assigned imhoffd and unassigned jgw96 Jul 11, 2018
@brandyscarney brandyscarney added this to the @ionic/angular 4.0.0 milestone Jul 11, 2018
@imhoffd imhoffd changed the title Proposal: follow the Angular 2 Style Guide more closely Proposal: follow the Angular Style Guide more closely Jul 12, 2018
@imhoffd
Copy link
Contributor

imhoffd commented Jul 30, 2018

Hi all! 👋 As many of you may have seen, we announced Ionic 4 beta on our blog! 🎉

Please update to the latest CLI and see our docs for getting started. A goal for Ionic 4 with Angular is to follow the Angular style guide as best we can. (Hopefully completely!) We welcome any and all feedback!

I'm closing this issue as I believe everything within it has been addressed with Ionic 4. If I'm mistaken, please create new issues in the CLI repo: https://github.com/ionic-team/ionic-cli

@imhoffd imhoffd closed this as completed Jul 30, 2018
@ionitron-bot
Copy link

ionitron-bot bot commented Sep 1, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests