Skip to content

Move app_title and browser_list to project.config #635

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

Closed
wants to merge 1 commit into from
Closed

Move app_title and browser_list to project.config #635

wants to merge 1 commit into from

Conversation

the-ult
Copy link
Contributor

@the-ult the-ult commented Mar 15, 2016

app_title and browser_list seem like project specific settings. Therefore, they should be defined in the project.config.ts instead of seed.config.ts

That way it is easier to overwrite seeds.config.ts every time there has been an update in angular2-seed.

Might even be an idea to create to following folders:

tools/
-- project
  -- config/
  -- tasks/
  -- utils/
-- seed
  -- config/
  -- tasks/
  -- utils/

That way you could easily update / clone angular2-seed again and just copy tools/seed to your project folder and you'll have all the fixes without having to compare it file to file.

@ludohenin
Copy link
Collaborator

Might even be an idea to create to following folders:

I tried this way when refactoring the tools and that's not ideal.
Anyway a utility will soon land to update from seed.

'android >= 4.4',
'bb >= 10'
];

constructor() {
super();
// this.APP_TITLE = 'Put name of your app here';
Copy link
Contributor

Choose a reason for hiding this comment

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

You can set APP_TITLE right here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah.. ok..
Still in the mindset -> const can't be changed. .(JAVA) which isn't true in ES6.
(And somehow blind.. cause I did not see that line.. :-s -> need more sleep)

But, I still think the APP_TITLE in project.config.ts would be more logical. (instead of overriding).

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it is consistent with the conventions in the rest of the seed.

i.e. Project overrides Seed.

That said, given the confusion around const and the intent of immutability vs. the reality of the implementation I do agree that APP_TITLE conveys the idea that it can't be modified when in reality, it can be but technically we aren't.

const is kind of a mess, in particular if you come from something like Java or .Net

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think you're correct with the consistency with the other conventions of the seed.

Other option could be a setter method for app_title which you can call from the project.config.ts constructor and other settings which are usually set in the project.config.ts but that sounds/feels messy as well.

What are the most overridden options besides APP_TITLE (and probably BROWSER_LIST)?

Copy link
Contributor

Choose a reason for hiding this comment

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

See #644

@mgechev
Copy link
Owner

mgechev commented Mar 18, 2016

@the-ult thanks for the contribution! Lets keep the title back in the seed config for now. It looks more consistent to me as well.

@mgechev mgechev closed this Mar 18, 2016
@the-ult
Copy link
Contributor Author

the-ult commented Mar 18, 2016

No problem 😊

Op vr 18 mrt. 2016 11:12 schreef Minko Gechev [email protected]:

Closed #635 #635.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#635 (comment)

met vriendelijke groet,
Arjen

M 06-51 800 419
W www.payt.nl

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