brunob / leaflet.fullscreen

Leaflet.Control.FullScreen for Leaflet
https://brunob.github.io/leaflet.fullscreen/
MIT License
376 stars 107 forks source link

fullScreenControlOptions.position changes position of zoom control #102

Closed php4fan closed 4 months ago

php4fan commented 2 years ago

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.

brunob commented 2 years ago

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);
php4fan commented 2 years ago

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.

brunob commented 2 years ago

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.

php4fan commented 2 years ago

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).

brunob commented 2 years ago

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 :

Does this plan reflect your thoughts ?

php4fan commented 2 years ago

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

Oh and also:

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.

BePo65 commented 11 months ago

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.

brunob commented 4 months ago

@php4fan would like to provide an update to document this in the readme ?

php4fan commented 4 months ago

no

php4fan commented 4 months ago

So nothing was changed in the end?