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

feat(aio): add marketing class to AppComponent <aio-shell> on mkt page #16395

Merged
merged 1 commit into from Apr 28, 2017

Conversation

wardbell
Copy link
Contributor

@wardbell wardbell commented Apr 28, 2017

Also navigation.json Doc page should be hidden in sidebar, shown in top-menu when top-menu is displayed in the sidenav (hidden now).

Sure these could be separate PRs. Both changes are tiny. Let's get on with it.

Remember to run yarn docs locally as this PR changes docs and navigation.json.


Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:
`

@googlebot googlebot removed the cla: no label Apr 28, 2017
@wardbell wardbell added this to REVIEW in docs-infra Apr 28, 2017
@wardbell wardbell added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 28, 2017
@wardbell wardbell self-assigned this Apr 28, 2017
@mary-poppins
Copy link

The angular.io preview for 682d093 is available here.

@petebacondarwin
Copy link
Member

These could be separate PRs, indeed, but if in a single PR they really should be separate commits no?

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

The changes to hidden stuff LGTM

The changes to adding the marketing css class would be simpler via a HostBinding I think:

@HostBinding('class.marketing') get isMarketing() { return !isSideNavDoc; }

@wardbell wardbell force-pushed the aio-add-marketing-class-to-shell branch from 682d093 to 5788038 Compare April 28, 2017 08:41
@petebacondarwin
Copy link
Member

Lovely! Please squash

@wardbell
Copy link
Contributor Author

wardbell commented Apr 28, 2017

I don't know if actually simpler (personally don't think so) but certainly more Angular idiomatic. Added in the 2nd commit.

Btw, I used an isMarketing field rather than a getter because (a) it seems more clear to me and (b) it is marginally more performant ;-)

I don't disagree with your point about separate commits ... having raised it myself ... but I've seen worse from absolutely everyone. Is it a blocker? I hope not.

@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 28, 2017
@wardbell wardbell force-pushed the aio-add-marketing-class-to-shell branch from 5788038 to 26b07e6 Compare April 28, 2017 08:49
Also navigation.json Doc page should be hidden in sidebar, shown in top
@wardbell wardbell force-pushed the aio-add-marketing-class-to-shell branch from 26b07e6 to dcf2add Compare April 28, 2017 08:49
@wardbell wardbell added action: review The PR is still awaiting reviews from at least one requested reviewer action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 28, 2017
@petebacondarwin petebacondarwin moved this from REVIEW to MERGE in docs-infra Apr 28, 2017
@mary-poppins
Copy link

The angular.io preview for 5788038 is available here.

@mary-poppins
Copy link

The angular.io preview for dcf2add is available here.

@gkalpak
Copy link
Member

gkalpak commented Apr 28, 2017

I've seen worse from absolutely everyone.

💔

@mhevery mhevery merged commit a2b2afb into angular:master Apr 28, 2017
@petebacondarwin petebacondarwin removed this from MERGE in docs-infra Apr 29, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
angular#16395)

Also navigation.json Doc page should be hidden in sidebar, shown in top
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
angular#16395)

Also navigation.json Doc page should be hidden in sidebar, shown in top
@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 11, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants