Skip to content

docs(getting-started): Getting Started Theme example #1580

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.

Sign up for GitHub

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
+11 −1
Closed

docs(getting-started): Getting Started Theme example #1580

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion GETTING_STARTED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,17 @@ export class PizzaPartyAppModule { }

## Including core and theme styles:
This is required to apply all of the core and theme styles to your application.
See the [theming guide](docs/theming.md) for instructions.
See the [Theming Guide](docs/theming.md) for instructions.

**From the Theming Guide:**

> If you're using Angular CLI, this is as simple as including one line in your `style.css` file:

> `@import '~@angular/material/core/theming/prebuilt/deeppurple-amber.css';`

> Alternatively, you can just reference the file directly. This would look something like:

> `<link href="node_modules/@angular/material/core/theming/prebuilt/indigo-pink.css" rel="stylesheet">`
Copy link
Member

@jelbourn jelbourn Oct 24, 2016

Choose a reason for hiding this comment

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

I'd rather not duplicate the content here and in the theming doc. You said you ran into this- why wasn't the link to the theming doc enough?

Copy link
Author

@mikeumus mikeumus Oct 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It's not just me. May you see the linked section of Gitter discussing this to see why?

TL:DR;

@DavidJohnWilliams, @grunzmeniroj, including the import in style.css got me too. It's because it's not in the Getting Started Guide.

A clear code example is what we're trained for/might be more effective here.

Copy link
Member

@jelbourn jelbourn Oct 24, 2016

Choose a reason for hiding this comment

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

Why wasn't the link to the theming doc enough? I'd rather be more emphatic in the this is required messaging than copy the content.

Copy link
Author

@mikeumus mikeumus Oct 24, 2016

Choose a reason for hiding this comment

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

It wasn't enough people seem to be missing it. It's probably good for the Getting Started to stand on it's own, even if we're not quoting exactly from the Theming Guide, something inline for people to get started without having to go other places.

Copy link
Member

@jelbourn jelbourn Oct 24, 2016

Choose a reason for hiding this comment

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

I'm trying to drill into the "people seem to be missing it." Did you not see the section since it's very short? Did you see it but thought it was optional?

Copy link
Author

@mikeumus mikeumus Oct 24, 2016

Choose a reason for hiding this comment

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

It got passed over as I think we're trained to look for a code example or something that looks more like a step to be taken. So maybe at least making it bold but the guide standing on it's own would be better.

Copy link
Contributor

@nkwood nkwood Oct 24, 2016

Choose a reason for hiding this comment

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

FWIW, although the doc does explicitly say "This is required to apply" and I'm not trying to defend people that don't read the docs: All of the previous headings before "Including core and themes and styles" are imperative statements ("Install" this.., "Create" that..), however the header for "Including core and theme styles" and all the headers after that ("Additional setup..", "Using Material"..) are passive informational headings (technically imperfect or continuous verbs). If one would just scan the headings, one might come to the incorrect conclusion everything after "Import the Angular Material NgModule" was optional.

I would suggest at least changing the header to "Include the required core and theme styles".

Also, if a statement similar to the one below was used, it would let someone accomplish the entirety of the "Getting Started" steps while staying on this page. I think from a usability / user experience standpoint there is a benefit to that. :

"For instance, if using Angular CLI, the required core and theme styles can be included simply by using a pre-built theme by including one line in your style.css file:
@import '~@angular/material/core/theming/prebuilt/deeppurple-amber.css';
See the theming guide for additional options.

My $.02.

Copy link
Member

@jelbourn jelbourn Oct 24, 2016

Choose a reason for hiding this comment

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

I sent PR #1588 to make the section more attention-grabbing. Going to close this for now, but we can re-assess after a few days.

Copy link

@DavidJohnWilliams DavidJohnWilliams Oct 25, 2016

Choose a reason for hiding this comment

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

I agree about duplication (although it's really only one line. Might I suggest changing it so the getting started guide says explicitly "Do this" with the one line code example. And then says "For more advanced theming see theming guide" and then just cut the one liner code out of the theming guide?



## Additional setup for `md-slide-toggle` and `md-slider`:
Expand Down