Skip to content

Conversation

mbehrlich
Copy link
Collaborator

Adds google-map-marker component.

@mbehrlich mbehrlich requested review from jelbourn and a team as code owners September 3, 2019 18:58
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 3, 2019
packages.bzl Outdated
@@ -82,6 +82,7 @@ MATERIAL_SCSS_LIBS = [

GOOGLE_MAPS_PACKAGES = [
"google-map",
"google-map-marker",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be a separate sub-package; I would just make it a sub-directory of google-map that is in the same build unit (e.g. no new tsconfig, NgModule, SystemJS config BUILD file, etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about this for awhile and I think you're right, that it should be in the same module. However, if that's the case, then we also shouldn't have a separate module for google-map. In other words, we should just have '@angular/google-maps' instead of '@angular/google-maps/google-map'. That's a pretty big refactor though, do you think it should be done in the same pull request?

Copy link
Member

Choose a reason for hiding this comment

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

That will be fine in a follow-up. I never actually realized it wasn't one main module for the whole thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, that sounds good. For this one, I will connect google-map-marker to the google-map module, and in the subsequent cl, I will have both of them be in a google-maps module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ran into some unexpected problems having google-map remain a separate subpackage but integrating map-marker. So, to avoid having to double my work, I decided to just combine everything into one package in this pr. It makes this already very large pr even bigger, but it should correct the build failures you saw.

@jelbourn
Copy link
Member

jelbourn commented Sep 6, 2019

@mbehrlich looks like there's a build failure on CI

Add a google-map-marker component that when used as a child to a
google-map component, will display a marker on the map.
Add tests for the google-map-marker component.
Add tests to Google Map marker component.
Fix bug with demo bazel file and with fake data for marker test.
Make various fixes internal cleanup fixes. Change google-map-marker
to map-marker. Use QueryList's changes property instead of a setter
for ContentChildren.
Fix type bug with map marker tests.
Refactor google-maps module to have a single entry-point,
@angular/google-maps, instead of a separate one for every component.
Fix formatting on google maps module.
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
Copy link
Member

jelbourn commented Sep 6, 2019

CI failure looks like you're just missing a module_name attribute on an ng_module rule.

@jelbourn jelbourn added pr: lgtm target: minor This PR is targeted for the next minor release labels Sep 6, 2019
Add GoogleMap and MapMarker components to index for the google maps
module.
@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker pr: merge safe labels Sep 6, 2019
@jelbourn jelbourn merged commit c8ceae5 into angular:master Sep 6, 2019
@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 Oct 7, 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: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants