herzbube / littlego

Little Go. An iOS application that lets you play the game of Go on the iPhone or iPad.
https://littlego.herzbube.ch/
Apache License 2.0
138 stars 53 forks source link

Make space between toolbar items flexible #356

Closed dingbat closed 3 years ago

dingbat commented 3 years ago

I made the space between the toolbar items flexible, but I also had to add a fixed width space equal to the current board position element's width due to it being a subview.

Another solution here would be to put the current board position into a bar button item with a custom view, but that would be more work and I wasn't sure if there was a particular reason you had decided to not do that. Can look into that if you prefer that solution, though.

Simulator Screen Shot - iPhone 8 - 2021-02-01 at 12 03 12

Fixes #346.

herzbube commented 3 years ago

Another solution here would be to put the current board position into a bar button item with a custom view

Once upon a time I did use this approach. Some historical digging turned up commit f7c4c9a where I decided to change things. The commit comment contains this:

until now the current BoardPositionView, and the ItemScrollView that lists all BoardPositionViews, were displayed as custom bar button items. this required all sorts of clumsy toolbar handling.

Looking at the commit details I see that I took out a number of spacers, but the "clumsy toolbar handling" comment probably was more targetted at the frame calculation stuff I had to do in viewWillLayoutSubviews. I have no idea if this would still be necessary these days - the commit comes from a time when iOS 7.1 was the newest SDK.

Can look into that if you prefer that solution, though.

I don't have a preference. The current PR is fine by me and I would merge it as soon as you give the good to go. But I also won't stand in your way - if you want to spend more time on this then feel free to investigate the "bar button item with a custom view" solution. Just don't forget to also test on iPad where the toolbar is also used.

dingbat commented 3 years ago

Ah cool, thanks for that context. I don't have a strong opinion, so I say we can leave it as is so the changeset can remain small. As for the toolbar on iPad, this is the before/after:

Screen Shot 2021-02-01 at 2 15 18 PM
Screen Shot 2021-02-01 at 2 21 36 PM

If this looks alright to you, I'm good with merging!