[Attribution] Show attribution present in TileJson source in attribution modal sheet #1087

ank27 · 2022-01-26T14:49:32Z

Summary of changes

if the user provides the source attribution in string literal (eg. {attribution: "OpenStreetMap contributors, CC-BY-SA"}), it doesn't populate in the attribution dialog, which should be shown. this pr fixes the issue.

Before	After

Pull request checklist:

Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. If tests were not written, please explain why.
Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
Add example if relevant.
Document any changes to public APIs.
Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Fix an issue where source attribution was not populated in attribution dialog.</changelog>.
If this PR is a v10.3 release branch fix / enhancement, merge it to main firstly and then port to v10.3 release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java

tobrun

Could we try optimizing the UI to make non-html attribution, not clickable? (setting them to disabled so they turn grey?)

pengdev · 2022-02-08T12:11:13Z

...attribution/src/main/java/com/mapbox/maps/plugin/attribution/AttributionDialogManagerImpl.kt

+          if (attribution.url.isEmpty()) {
+            setTextColor(Color.GRAY)
+          } else {
+            setTextColor(ContextCompat.getColor(context, R.color.mapbox_blue))


is it necessary to set the blue color again? I think the default style defined in mapbox_attribution_list_item is mapbox_blue already.

yes, somehow the color GRAY was set to whole list, so added the condition.

pengdev

LGTM % nit

kiryldz · 2022-02-08T13:41:00Z

sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java

      attributionParser.createAttributionString(true)
    );
  }
+
+  @Test
+  public void testParseAttributionWithCustomStringAttribution() throws Exception {


pretty unexpected to see test in Java here 😄

We need to convert it to kotlin, probably later :D

sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java

kiryldz · 2022-02-08T13:45:58Z

...attribution/src/main/java/com/mapbox/maps/plugin/attribution/AttributionDialogManagerImpl.kt

+        val attribution = attributions[position]
+        view.findViewById<TextView>(android.R.id.text1).apply {
+          // if attribution url is empty, we show them as disabled.
+          if (attribution.url.isEmpty()) {


nit: could put if inside:

setTextColor(if () ... else ...)

kiryldz · 2022-02-08T13:51:54Z

sdk/src/main/java/com/mapbox/maps/attribution/AttributionParser.kt

+  /**
+   * Parse string literal to attribution with empty url.
+   */
+  private fun parseStringLiteralToAttributions(stringLiteralArray: MutableList<String>) {


is indeed a mutable list needed here?

additionally naming stringLiteralArray with word array is not aligned well with actual list type

yeah, I will update that.

kiryldz

Some minor suggestions however dropping 🟢 in order not to block merging after those will be fixed.
Also missing checkpoint to port this to v10.3 branch - is this intended? I thought we want to have it in v10.3 stable however I don't have strong opinion here, imo it could wait until v10.4-beta.1

…Test.java Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>

ank27 · 2022-02-08T14:01:52Z

Some minor suggestions however dropping 🟢 in order not to block merging after those will be fixed. Also missing checkpoint to port this to v10.3 branch - is this intended? I thought we want to have it in v10.3 stable however I don't have strong opinion here, imo it could wait until v10.4-beta.1

Yes, added the check. this will be included in v10.3

…ion modal sheet (#1087) * allow string attribution * Update sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com> * update attribute parser Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>

…ion modal sheet (#1087) (#1129) * allow string attribution * Update sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com> * update attribute parser Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com> Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>

ank27 added the bug 🪲 Something isn't working label Jan 26, 2022

ank27 self-assigned this Jan 26, 2022

ank27 marked this pull request as ready for review January 27, 2022 12:29

ank27 requested a review from a team as a code owner January 27, 2022 12:29

pengdev reviewed Jan 27, 2022

View reviewed changes

sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java Show resolved Hide resolved

tobrun reviewed Feb 1, 2022

View reviewed changes

allow string attribution

71a8727

ank27 force-pushed the ak-allow-custom-attribution branch from bbb3593 to 71a8727 Compare February 8, 2022 10:52

ank27 requested review from tobrun and pengdev February 8, 2022 10:52

pengdev reviewed Feb 8, 2022

View reviewed changes

pengdev approved these changes Feb 8, 2022

View reviewed changes

kiryldz reviewed Feb 8, 2022

View reviewed changes

sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java Outdated Show resolved Hide resolved

kiryldz reviewed Feb 8, 2022

View reviewed changes

sdk/src/test/java/com/mapbox/maps/attribution/AttributionParseTest.java Outdated Show resolved Hide resolved

kiryldz reviewed Feb 8, 2022

View reviewed changes

kiryldz approved these changes Feb 8, 2022

View reviewed changes

Update sdk/src/test/java/com/mapbox/maps/attribution/AttributionParse…

adf79a5

…Test.java Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>

update attribute parser

061b830

ank27 force-pushed the ak-allow-custom-attribution branch from 76ea168 to 061b830 Compare February 8, 2022 14:13

ank27 merged commit 952832b into main Feb 8, 2022

ank27 deleted the ak-allow-custom-attribution branch February 8, 2022 14:47

ank27 mentioned this pull request Feb 8, 2022

[port] Show attribution present in TileJson source in attribution #1087 #1129

Merged

9 tasks

evil159 mentioned this pull request Mar 2, 2022

Add support for non-HTML attribution mapbox/mapbox-maps-ios#1163

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Attribution] Show attribution present in TileJson source in attribution modal sheet #1087

[Attribution] Show attribution present in TileJson source in attribution modal sheet #1087

ank27 commented Jan 26, 2022 •

edited

Loading

tobrun left a comment

pengdev Feb 8, 2022

ank27 Feb 8, 2022

pengdev left a comment

kiryldz Feb 8, 2022

ank27 Feb 8, 2022

kiryldz Feb 8, 2022

kiryldz Feb 8, 2022

kiryldz Feb 8, 2022

ank27 Feb 8, 2022

kiryldz left a comment

ank27 commented Feb 8, 2022

[Attribution] Show attribution present in TileJson source in attribution modal sheet #1087

[Attribution] Show attribution present in TileJson source in attribution modal sheet #1087

Conversation

ank27 commented Jan 26, 2022 • edited Loading

Summary of changes

Pull request checklist:

tobrun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pengdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiryldz left a comment

Choose a reason for hiding this comment

ank27 commented Feb 8, 2022

ank27 commented Jan 26, 2022 •

edited

Loading