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

Please start providing context for nuanced config settings. #6068

Closed
wants to merge 1 commit into from
Closed

Please start providing context for nuanced config settings. #6068

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 26, 2017

Every config I have seen lacks context. When dealing with complex configs, we need to start providing the settings around the configuration options of interest in case other passer-bys are looking for where to include these specific and nuanced settings. This is the only page that talks about scss being used in angular-cli.json

Every config I have seen lacks context.  When dealing with complex configs, we need to start providing the settings around the configuration options of interest in case other passer-bys are looking for where to include these specific and nuanced settings.  This is the only page that talks about scss being used in angular-cli.json
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 26, 2017
@ghost ghost mentioned this pull request Jul 26, 2017
@ghost
Copy link
Author

ghost commented Aug 7, 2017

@mmalerba Why is travis-ci linter failing? This isn't a code change it's a documentation change.

@mmalerba
Copy link
Contributor

mmalerba commented Aug 7, 2017

@megamindbrian looks like a lint error that was accidentally merged into master a while back. It has since been fixed and will probably go away if you rebase

@ghost
Copy link
Author

ghost commented Aug 8, 2017

@mmalerba How do I rebase a commit made through Githubs editor? All the changes are stored online so I don't have a separate branch. How do I checkout megamindbrian:patch-1 when it doesn't show in the history?
Looks like you can do this (I don't have the authority):
image

@mmalerba
Copy link
Contributor

mmalerba commented Aug 8, 2017

I'll just ignore the lint issue warning merge after this is LGTM'd, since it's clearly unrelated to your changes

@ghost
Copy link
Author

ghost commented Aug 8, 2017 via email

@jelbourn
Copy link
Member

I don't think this is the place to describe what the config file should look like; that level of detail belongs to the angular-cli documentation

@jelbourn jelbourn closed this Aug 17, 2017
@ghost
Copy link
Author

ghost commented Aug 17, 2017 via email

@willshowell
Copy link
Contributor

If you are using the Angular CLI, support for compiling Sass to css is built-in; you only have to add a new entry to the "styles" list in angular-cli.json pointing to the theme file (e.g., unicorn-app-theme.scss).

@megamindbrian what context is missing? That paragraph tells you

  1. What entry you need to add
  2. What file you need to add it to
  3. Where in that file you should add it

The only improvement I could see is changing angular-cli.json to .angular-cli.json to match recent cli updates.

@ghost
Copy link
Author

ghost commented Aug 17, 2017 via email

@willshowell
Copy link
Contributor

in angular-cli.json

@ghost
Copy link
Author

ghost commented Aug 17, 2017

What entry you need to add
What file you need to add it to
Where in that file you should add it

@willshowell The current instructions don't represent the final result. Those 3 statements in 1 sentence don't look like a configuration. No matter what your argument is, a sentence:

If you are using the Angular CLI, support for compiling Sass to css is built-in; you only have to add a new entry to the "styles" list in angular-cli.json pointing to the theme file (e.g., unicorn-app-theme.scss).

Does not look like a config change. Why shouldn't documentation follow the same SOLID principles that code does? Documentation should look like the expected result.

{
    "apps": [
        {
            "name": "unicorn-app",
            ... main, polyfills, etc
        },
        "styles": [
            "unicorn-app-theme.scss"
        ]
    ]
}
{
    "apps": [
        {
            "name": "unicorn-app",
            ... main, polyfills, etc
        }
    ],
    "styles": [
        "unicorn-app-theme.scss"
    ]
}
{
    "defaults": {
        "styles": [
            "unicorn-app-theme.scss"
        ]
    }
}
{
    "project": {
        "styles": [
            "unicorn-app-theme.scss"
        ]
    }
}

When I scroll though the page and I see a code block, I think "Oh! That must be a config change!". When I scroll through and I see 1 line out of hundreds that describes the name of the file, that line does not look like a config change. That looks like some arbitrary instructions. You could even number the steps if that would be better.

@ghost
Copy link
Author

ghost commented Aug 17, 2017

#6530

@ghost
Copy link
Author

ghost commented Aug 18, 2017

#4125 (comment)

@p3x-robot
Copy link

p3x-robot commented Aug 18, 2017

this scss is still not fixed. :( no matter i do i get this, i use 9 themes, all dynamic, the aot built no warning but in dev it shows warning and i do use the @mat-core, i even include every where.

does it only work angular-cli or what, i use the exact theme guide , still the warning keeps.
what can i do? i see my questions is closed, but the "warning" kept.
ok it works but ok, why do i get a warning without i created the themes guide.

i just use webpack. that's all plus angular-compiler, with or without styleUrls....

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants