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

some BasemapLayers write 'null' to Leaflet's attribution control #647

Closed
jgravois opened this issue Sep 28, 2015 · 8 comments

Comments

@jgravois
Copy link
Contributor

@jgravois jgravois commented Sep 28, 2015

when adding a 'Labels' layer to the map (OceansLabels, GrayLabels, DarkGrayLabels), 'null' is added (as a string) to Leaflet's attributionControl. see the bottom righthand corner of the screenshot below for an example.

screenshot 2015-09-28 12 49 43

to reproduce this bug:

  1. fork/clone the repo
  2. open debug/sample.html and substitute the following basemapLayers
L.esri.basemapLayer('Oceans').addTo(map);
L.esri.basemapLayer('OceansLabels').addTo(map);
  1. open the sample application in a browser to confirm that the word 'null' is included.

this is happening because we currently pass an actual null to the L.esri.basemapLayer addAttribution() method here. bonus points if you're able to write a new test which checks for this kind of problem in the future.

with inspiration from the folks at @yourfirstpr and 'First Timers Only' by @kentcdodds, we are looking for a fix from someone that is just getting started with contributing to open source. if you're interested, but need help getting started, please don't be shy!

@jgravois jgravois changed the title Label BasemapLayers write 'null' to the attribution control some BasemapLayers write 'null' to Leaflet's attribution control Sep 28, 2015
@kentcdodds

This comment has been minimized.

Copy link

@kentcdodds kentcdodds commented Sep 28, 2015

👏 👏 👏 👏 Glad to see first-timers-only taking off.

If you're a first timer, take advantage of this invitation. It's an invitation to get some hand holding on your first PR. Bravo @jgravois. It definitely takes more work to hold someone's hand through this, but it's totally worth it :-) 👏 👏 👏 👏

@brianbancroft

This comment has been minimized.

Copy link
Contributor

@brianbancroft brianbancroft commented Oct 7, 2015

Hi there, I thought I solved the problems with my installation, but that was not the case. I'm going to try to jump into that later, but after looking at the real problem, it turned out to be an easier fix than I was expecting, being my first kick at this can.

In this particular instance of the problem, I found that the OceanLabels class lacked an attribution value, and that's why the addAttribution() function pulls a null. If this is the correct path to go with the problem, then would adding attribution to the remainder of the classes in this file be the correct path of action?

As far as the test, I believe that dropping a if/else loop that verifies whether the function pulls a null value. Right now, I'm testing something like the following:

  getAttribution: function () {
    if (this.options.attribution) { 
        var attribution = '<span class="esri-attributions" style="line-height:14px; vertical-align: -3px; text-overflow:ellipsis; white-space:nowrap; overflow:hidden; display:inline-block;">' + this.options.attribution + '</span>';
} else { 
    var attribution = '<span class="esri-attributions" style="line-height:14px; vertical-align: -3px; text-overflow:ellipsis; white-space:nowrap; overflow:hidden; display:inline-block;">' + 'Attribution missing' + '</span>';
    return attribution;
  },

@jgravois

This comment has been minimized.

Copy link
Contributor Author

@jgravois jgravois commented Oct 7, 2015

awesome work.

I don't even think we need an else block. we just need to make sure not to return attribution unless it's present in the service itself.

its valid that the labels layers don't have attribution (otherwise we'd just end up with duplicate references to the data sources used for the paired layers)

@brianbancroft

This comment has been minimized.

Copy link
Contributor

@brianbancroft brianbancroft commented Oct 9, 2015

So I've run into another series of first-timer issues. The idea of using the if loop should work, but when I've been attempting it locally, it hasn't been that lucky. The null still appears - even when I put a dummy attribution in the OceansLabels base feature class.

To troubleshoot further, I also used a statement guaranteed to return a false value to the if statement if(4<1), which produced no change in the result either! This doesn't make sense to me. My assumption is that getAttribution() is called up for each base layer when the page is loaded. If this is correct, then I can think of two cases of how this went wrong, but I don't understand why I received my results in either case:

  • The first case assumes that I didn't botch the syntax. Is there any reason why you have attribution on the lower-right if the getAttribution() returns nothing?
  • The second case is that I did botch up the syntax. Does the web browser keep an older version of the .js file cached in case the new one doesn't work?

Thanks for the encouragement and assistance so far!

@jgravois

This comment has been minimized.

Copy link
Contributor Author

@jgravois jgravois commented Oct 9, 2015

at this point, it'd be easiest to discuss the proposed change in more detail if you submitted a pull request so I can see the exact differences in your code and perhaps even test them locally.

with regard to the strange behavior in your browser...

  1. for now you need to make sure to call npm run build to recompile the minified source each time you make a change.

  2. I've noticed chrome in particular sometimes continues to load stale content even after I manually cleared the browser cache. it's a lot easier to see whether you've loaded the latest changes if you have a breakpoint set in the unminified sourcemapped code in the browser's developer tools. a full restart (of the browser) has always cleared things up for me.

@brianbancroft

This comment has been minimized.

Copy link
Contributor

@brianbancroft brianbancroft commented Oct 10, 2015

Ah, the npm run build was the solution to the problem I was having. Thanks again!

I've created a pull request with what I've verified on Chrome and Firefox to be a solution to the bug.

Once again, thanks for taking me on for this easy fix. I've learned a lot about git, command line that I wouldn't have otherwise in my spare time!

@kentcdodds

This comment has been minimized.

Copy link

@kentcdodds kentcdodds commented Oct 10, 2015

That's exactly what this first-timers-only initiative is all about @brianbancroft. I'm so glad you had a good experience. Thanks to @jgravois for being willing to help! :D This is awesome :D

jgravois added a commit that referenced this issue Oct 14, 2015
A solution to issue #647
@jgravois

This comment has been minimized.

Copy link
Contributor Author

@jgravois jgravois commented Oct 14, 2015

resolved by #651. well done @brianbancroft!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.