leaflet.fullscreen icon indicating copy to clipboard operation
leaflet.fullscreen copied to clipboard

fullScreenControlOptions.position changes position of zoom control

Open php4fan opened this issue 3 years ago • 7 comments

With this:

var map = L.map(mapElement, {
    //...
    fullscreenControl: true,
    fullscreenControlOptions: {
        position: 'bottomright'
    },
        
    zoomControl: true,
    zoomControlOptions: {
        position: 'topleft'
    },
    //.....    
};

Expected: The zoom control should be on the top left and the fullscreen control should be on the bottom right.

Observed Both are on the bottom right.

php4fan avatar Mar 03 '22 22:03 php4fan

You should use forceSeparateButton an add fullscreen control manually to achieve that, cf :

L.control.fullscreen({
  position: 'bottomright', // change the position of the button can be topleft, topright, bottomright or bottomleft, default topleft
  forceSeparateButton: true, // force separate button to detach from zoom buttons, default false
}).addTo(map);

brunob avatar Mar 05 '22 15:03 brunob

Ok so there is a workaround (poorly documented if at all), but it's still a bug. Or a bad design which is just another type of bug.

There's no need for a forceSeparateButton at all. If I don't specify a position for your control, then it makes sense to put it by default together with the zoom control, and if there is no zoom control, at the topleft. It's a nice default.

If I do specify a position for your control but I don't specify any position for the zoom control, it's pretty obvious that I want the zoom control to be at its default position (which is topleft) and yours at whatever position I have specified. If those are not the same position, then forceSeparateButton should be assumed in this case. It's nonsensical and counterintuitive (and not documented) that the position that I specify for your control becomes also the position of the zoom control.

And even more so if I do specify both positions explicitly and they are different. If I say I want the zoom control on the topleft and yours at bottom right, then obviously they have to be separated.

php4fan avatar Mar 05 '22 16:03 php4fan

Ok so there is a workaround (poorly documented if at all), but it's still a bug. Or a bad design which is just another type of bug.

U're really welcome, I'm so pleased to help you :)

If I don't specify a position for your control, then it makes sense to put it by default together with the zoom control, and if there is no zoom control, at the topleft. It's a nice default.

I think this kind of automagic behavior is not the a way to help people understand your, my or ervery one else "logic".

Anyway, feel free to provide a PR if you think this script or the doc could be better.

brunob avatar Mar 05 '22 16:03 brunob

I think this kind of automagic behavior is not the a way to help people understand your, my or ervery one else "logic".

I'm not sure what you mean by that. I'm describing what the behavior should be (and there's nothing automagic about it), from a user standpoint regardless of implementation details or difficulties. Is my description of the expected behavior unclear or do you disagree that that is what one should expect?

Is the current behavior the way it is just because it was easier to implement, or do you have arguments to support the superiority of the current behavior?

U're really welcome, I'm so pleased to help you :)

Oh sorry I forgot to say how grateful I am that you developed this amazing library and share it for free. I thought that was understood (otherwise I wouldn't take the time to come here and report a bug or suggest an improvement).

php4fan avatar Mar 05 '22 16:03 php4fan

Oh sorry I forgot to say how grateful I am that you developed this amazing library and share it for free.

Nice or sarcastic... let's say it's nice :)

Before we try to work together to make things better, i think we should :

  • look at how the official (and unsuppported) plugin work https://github.com/Leaflet/Leaflet.fullscreen/blob/gh-pages/dist/Leaflet.fullscreen.js
  • plan what we want to do for the present script
    • defautl position is topleft
      • if there isn't zoom control, add a standalone (forceSeparateButton) FS control
      • if there is a zoom control, add FS control linked to it
    • if position is different than topleft and this position is different than the one specified for zoom control, add a standalone FS control
  • in order to keep backward compatibility, if forceSeparateButton is specified, use it in any case

Does this plan reflect your thoughts ?

brunob avatar Mar 05 '22 19:03 brunob

I would rather describe it as follows (and I don't think it's exactly the same):

  • if there isn't a zoom control, add a standalone FS control.
    • in which case, if a position for FS is specified, use that, otherwise use topleft by default
  • if instead there is a zoom control, then:
    • if no position for FS is specified, put it below the zoom control (I guess "linked to" zoom control?), whatever its position is (it may or may not be topleft, it may be set explicitly or by Leaflet's defaults, it doesn't matter)
    • if a position for FS is specified AND it's different from the zoom control's position (again, this is not necessarily topleft), then put the FS as a standalone control at the specified position
    • if a position for FS is specified and it is the same as the one of the zoom control (again, may or may not be topleft), then put it below the zoom control.

Oh and also:

  • whatever the combination of settings is, I should be able to either specify them as map options on map creation, or add the control manually with addTo().
  • If for whatever reason that's not possible, then I'd seriously consider dropping the options-on-map-creation option completely. I would rather be forced to use addTo to begin with, than have the false impression of simplicity and then be forced to switch to using it when I find out that the first model only covers a subset of options.
  • If, however, what I just said is a bad idea and you opt for supporting a map-option syntax that only covers a subset of cases, to make the plugin easier to use for all-default or mostly-default usage, then one thing that you really should avoid at all cost is that a FS-specific setting (like fullScreenControlOptions.position or whatever it was) changes the position of the Zoom Control (neither when the latter is not set explicitly, nor even worse when it is). That is not acceptable under any circumstances.

This is based on the assumption that you want to maintain the loosely-defined "under the Zoom control by default" and "topleft by default if standalone" design decisions that seem to be the starting point of your design. I don't have a strong opinion on whether or not that should be the default. It's one reasonable default. If it was up to me, i would suggest to put the FS on the bottom-right by default (therefore separate by default). That is where the fullscreen control of most fullscreen-able stuff usually is (most video players, for example). And it is completely unrelated to the zoom control anyway. And then I would question: does it even make sense to "link or not link" the FS control to the zoom control? To me, they are two distinct, unrelated controls. They either are in different positions or in the same position (where by position I mean topleft, bottomright, etc). If they are in the same position then the question arises of how to arrange them to not have a conflict: one on top of the other, one next to the other, which one on top or which one first, etc. That sounds to me like a general problem when several controls have the same position. Doesn't Leaflet already have a framework for that? (genuinely asking, I don't know). Is there anything to be gained by "linking" the control to another one?

in order to keep backward compatibility, if forceSeparateButton is specified, use it in any case

I don't think it's possible to maintain complete BC while at the same time honoring the behavior described above. If you mean keep the most possible BC, I guess whatever you deem necessary. I really don't know what meaning forceSeparateButton (and therefore "use it in any case") can have in the context of the new behavior. If the position of the two controls is different, then forceSeparate would have no effect anyway. If it's the same position, what effect would forceSeparate have? Do you mean force them to be separate controls in the same position? Again, I don't know how Leaflet normally treats two controls that have the same (topleft|bottomright|etc) position: if it allows you to arrange them nicely, then I don't see the need for ever having them not separated.

php4fan avatar Mar 05 '22 21:03 php4fan

Let me take some arguments of @php4fan and create a list of what happens when we create a fullscreen control with different options:

  1. by default the fullscreen button is added to the zoomControl (see README.md right after the second gray box in How?)
  2. If you don't have a zoomControl the button will be created in a container of its own. The default position of this container is the default position of leaflet controls - the topleft corner of the map (see README.md same position).
  3. If you want the button in a separate container use the forceSeparateButton option (see the api documentation in the README.md).
  4. If you want to modify the position of the button container use the position option (see the api documentation in the README.md).

The only point that may be surprising is the fact that the position option will even do its work, if you add the fullscreen button to the zoomControl. In that case the position of the zoom-Control with the added fullscreen button is changed.

I think that it wold be a good idea, to add this fact to the readme.

But I can see no bugs in the handling of the options,; just some design decisions - that you may agree to / like or not.

BePo65 avatar Oct 12 '23 05:10 BePo65