Skip to content

Schematic fixes #10388

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

Merged
merged 5 commits into from
Mar 14, 2018
Merged

Schematic fixes #10388

merged 5 commits into from
Mar 14, 2018

Conversation

amcdnl
Copy link
Contributor

@amcdnl amcdnl commented Mar 13, 2018

Addresses items in #10352

  • Fixes link tags poorly formatted when inserted into index.html
  • Addsbody { margin: 0; } on initial scaffold
  • Fixes changed after checked error
  • Fixes data not showing up in table after page/sort
  • Fixes sidenav in inverse state
  • Switches == to ===

@amcdnl amcdnl self-assigned this Mar 13, 2018
@amcdnl amcdnl requested a review from jelbourn March 13, 2018 02:26
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 13, 2018
@amcdnl amcdnl mentioned this pull request Mar 13, 2018
});

if (!styles) {
throw new SchematicsException(`Could not find global styles.ext file.`);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it valid to call your root style file something else entirely? Can we read the first stylesheet from the config? Would it be better to just no-op if we can't find it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well there is just a styles array where this lives which could include other stuff too. They could change it but I find it HIGHLY unlikely. I'll noop more gracefully.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I know if I was making a large app, I wouldn't want to have a file called styles.css because it's too vague, so I'd rename it to something like global-styles.css or browser-reset.css

const config = getConfig(host);
const app = getAppFromConfig(config, '0');
const styles = app.styles.find(style => {
const str = style.toString();
Copy link
Member

Choose a reason for hiding this comment

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

const styles = app.styles.find(s => /styles\.(c|le|sc)ss/.test(s));

Copy link
Contributor Author

@amcdnl amcdnl Mar 13, 2018

Choose a reason for hiding this comment

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

Fancy ;P

`<link href="https://fonts.googleapis.com/css?family=Roboto:300,400,500" rel="stylesheet">`);
addHeadLink(host,
`<link href="https://fonts.googleapis.com/icon?family=Material+Icons" rel="stylesheet">`);
addHeadLink(host, `
Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer to not rely on newlines in template strings since it's not immediately obvious that they're intentional and someone might remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, I agree. Got a better alternative? Just /n?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, just \n is obvious that it's intentional

@amcdnl
Copy link
Contributor Author

amcdnl commented Mar 14, 2018

@jelbourn - done.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Mar 14, 2018
@mmalerba mmalerba merged commit e3d2e14 into angular:master Mar 14, 2018
@amcdnl amcdnl deleted the schematic-fixes branch March 14, 2018 23:23
josephperrott pushed a commit to josephperrott/components that referenced this pull request Mar 19, 2018
* fix(schematics): better formatting on index, insert body margin 0

* fix(schematics): fix wrong mode on init

* fix(schemtic): convert == to ===

* fix(schematic): fix check expression

* chore: pr feedback
@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
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants