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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add StyleSpans support for Polylines #546

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

el-qq
Copy link
Contributor

@el-qq el-qq commented Apr 10, 2024

Fixes #307 馃

There's a similar PR that was proposed some time ago. Unfortunately, the author has not responded or continued working on it.
Therefore, created a similar PR that takes into account the latest changes from the comments.


Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@el-qq el-qq changed the title feat: Add StyleSpans support for Polylines # feat: Add StyleSpans support for Polylines Apr 10, 2024
@@ -106,4 +213,4 @@ public fun Polyline(
set(zIndex) { this.polyline.zIndex = it }
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that all these set() calls should instead be update(), for reasons of performance & tidiness. It would be helpful to reflect this concern somewhere, but I'm not a maintainer. Could you create a separate issue for this? Or fold it into this one, but it would seem to mix different concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GoogleMapComposable
private fun PolylineImpl(
points: List<LatLng>,
spans: List<StyleSpan> = emptyList(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like PolylineImpl() could be eliminated entirely in favor of having a single Polyline() method with a default parameter. It's a bit of a breaking change, but personally I'd favor it over having so much repetitive overload verbosity, just like with the Marker() overloads nightmare. I'm not a maintainer, though.

Copy link
Contributor Author

@el-qq el-qq Apr 10, 2024

Choose a reason for hiding this comment

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

Then there will be confusion with the color parameter - it makes no sense to specify it when we specify Spans for segments

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed on keeping both separate, since we have spans on a constructor and color on another.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@el-qq
Copy link
Contributor Author

el-qq commented Apr 22, 2024

updated the documentation on the comments

@kikoso
Copy link
Collaborator

kikoso commented Apr 22, 2024

Thanks @el-qq ! This looks good.

@alexandrupele
Copy link

guys thank you so much for adding this, i need it in my project. can't wait to get it released

@el-qq
Copy link
Contributor Author

el-qq commented Apr 23, 2024

@dkhawk merge?
As I understand it, the problem with the instrumentation tests is general and outside the scope of this issue

@kikoso
Copy link
Collaborator

kikoso commented Apr 23, 2024

@el-qq , the Instrumentation Tests fail since the runner in your machine does not have access to the Maps API Key, this is expected from this PR.

We are currently evaluating this feature.

@dkhawk dkhawk merged commit 105112a into googlemaps:main Apr 23, 2024
8 of 9 checks passed
googlemaps-bot pushed a commit that referenced this pull request Apr 23, 2024
# [4.4.0](v4.3.5...v4.4.0) (2024-04-23)

### Features

* Add StyleSpans support for Polylines ([#546](#546)) ([105112a](105112a))
@googlemaps-bot
Copy link
Contributor

馃帀 This PR is included in version 4.4.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support of StyleSpan in Polylines
6 participants