Add Fullscreen to any map

[baldulin]

I wanted to use the requestFullscreen api to show the map fullscreen. I know it is possible to do this without altering pigeon-maps but I think it is cleaner to implement it into the map directly.

I also have some other features I needed so I would welcome your general feedback. Because I might open more pull requests later on.

Edit: The original branch of this pull request is now here https://github.com/baldulin/pigeon-maps/tree/oldImplementFullscreen

[baldulin]

Hey,

sorry that it bugged figured out the bug and fixed it but moved it to this branch. Didn't provide the correct props.

I see your points and they are reasonable. I tried to reimplement the feature according to your wishes.

• The button is now it's own component under controls/FullscreenControl. But for the ZoomControl and Fullscreen Control buttons to interact in a good manner I had to change their positioning to relative. Which does not interfere with ordering of children of the map (if not altering the default styles at least). The only other possibility I see to do this is add an ControlPanel component that can be used to wrap ZoomControl and FullscreenControl either internally/implicitly in the Map component or explicitly.

• The button has now a prop container that can be used to change the node that is fullscreened (the white fullscreen button in the demo does this). I guess one could misuse this to fullscreen something that is either a child component of the map or in a different branch than the map component, guess users will know that to do this is stupid. If undefined the mapContainer is used (for this to be possible I had to add the _containerRef to the overlay props)

• It has a prop isFullscreen that can be undefined than the button takes care of the state management or it's either true or false, in which case the state must be handled by the user. I guess this fixes your redux concern. I added a button to show this as well in the demo

There is an issue right now with the height management and I'm not sure what the point there is, if you click the red fullscreen button in the demo you will see that once you exited fullscreen the map height will remain the height of your screen. This is due to the fact that the size event will overwrite the value set in the height prop, I'm not really sure how to fix that.

[baldulin]

Just noticed that the marker will be moved by position relative. As transform moves them away from the initial position, thus they explicitly need to be set an initial position. I guess you use the transform property because of animations so I just added the left/top attributes to the three components with a value of 0.

[baldulin]

Okay sorry for the long wait.

I think this thing turned way more messy (and maybe even flaky) than I wanted it to be. Maybe this is just the way fullscreen mode works. So any recommendations to cleanup this act are really welcome.

1. I think this was caused by an effect that forced the map back into fullscreen if it was exited by "escape", oddly enough firefox prevents certain fullscreen requests. So this never happend at my end.

2. The height thing also happens if one sets the width. And this is caused by the fact that the map explicitly gets set it's own width. To fix this bug I had to add a lot of code in the Map component. Basically I need to store the height before going to fullscreen and toggle back to it after exiting fullscreen mode.

3. I used some basic CC0 icons I don't like them to be honest the unicode ones look better but I'll ask a friend to help me on this. And they are replaceable now.

4. Maybe like this it's fine. To have multiple controls now one needs the ControlContainer. But using one type of control will work fine without.

[mariusandra]

Hey @baldulin, I really appreciate all the effort you have put into it. However I'm going to have to reject the PR. Like you said yourself, it's quite flakey. And since I'm the one who will need to support this library and it's bugs for all eternity, I strongly prefer a small yet stable and extendable library to a bigger and flakier one.

Unfortunately, this adds a lot of tightly copled code to the core, and there's no way to make it better.

Like I wrote earlier:

However, I'm not sure if this is the right thing to add to the core of the library, as this is just one of many ways somebody might want to make their app full screen.

Full screen is not a simple problem. No matter what you do, some or many people want it differently. It all depends on your app, how it's wired and what it needs.

Taking all of that into account, I strongly believe this is something that should be handled by client code, not library.

Thanks again for all the work. Really appreciate it!

[baldulin]

Yeah I see your point. I try to figure out a way to implement this outside of the Map component. But I still think that some minor changes to the Map component would be great to implement features like this. Let's see