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): enable data driven homepage announcements #22043

Closed

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Feb 6, 2018

No description provided.

private findCurrentAnnouncement(announcements: Announcement[]) {
return announcements
.filter(announcement => new Date(announcement.startDate).valueOf() < Date.now())
.filter(announcement => new Date(announcement.endDate).valueOf() > Date.now())
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@Component({
selector: 'aio-announcement-bar',
template: `
<!-- Announcement Bar -->
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is no longer needed

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release and removed state: WIP labels Feb 7, 2018
@petebacondarwin petebacondarwin moved this from WIP to REVIEW in docs-infra Feb 7, 2018
"message": "Join us in Atlanta for ngATL<br/>Jan 30 - Feb 2, 2018",
"imageUrl": "generated/images/marketing/home/ng-atl.png",
"linkUrl": "http://ng-atl.org/"
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This ngATL banner will never be shown as it has expired, but I am leaving it here as an example.

I have not yet added the ng-conf banner because the logo doesn't look right.

@@ -34,7 +35,7 @@ import { ResourceService } from './resource/resource.service';
* such as CodeExampleComponent, LiveExampleComponent,...
*/
export const embeddedComponents: Type<any>[] = [
ApiListComponent, CodeExampleComponent, CodeTabsComponent, ContributorListComponent,
AnnouncementBarComponent, ApiListComponent, CodeExampleComponent, CodeTabsComponent, ContributorListComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not quite right. If we want to get good experience for loading the homepage, we need to bundle the AnnouncementBarComponent within the main bundle. Otherwise we won't be able to fully render the homepage without downloaded the main bundle and the embedded components bundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! So the two options are:

  1. Move the announcements component to the main bundle so it is loaded upfront.
  2. Move the announcement component to its own bundle, so we don't need to load the other embedded components on the homepage.

The former is simpler but makes the main bundle larger even if someone is going straight to the docs pages. The latter requires a second request, but this could be HTTP preloaded for the homepage.

Copy link
Member Author

Choose a reason for hiding this comment

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

which do you prefer @IgorMinar ?

@IgorMinar
Copy link
Contributor

IgorMinar commented Feb 8, 2018 via email

@petebacondarwin
Copy link
Member Author

Done - PTAL

@mary-poppins
Copy link

You can preview ee64148 at https://pr22043-ee64148.ngbuilds.io/.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

thanks Pete!

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 9, 2018
@mhevery mhevery closed this in fbef94a Feb 9, 2018
mhevery pushed a commit that referenced this pull request Feb 9, 2018
@gkalpak gkalpak removed this from REVIEW in docs-infra Feb 15, 2018
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
@petebacondarwin petebacondarwin deleted the aio-announcement-bar branch February 25, 2018 09:56
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@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 13, 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 feature Issue that requests a new feature target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants