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

Night mode #69

Merged
merged 118 commits into from May 8, 2019
Merged

Night mode #69

merged 118 commits into from May 8, 2019

Conversation

mltbnz
Copy link
Member

@mltbnz mltbnz commented Apr 10, 2019

Night Mode

  • adds Light and DarkTheme
  • stores selected Theme in UserDefaults
  • adds switch in settings to set theme

Checklist

Before creating the PR

  • Update CHANGELOG

@mltbnz mltbnz added the wip work in progress label Apr 10, 2019
@mltbnz
Copy link
Member Author

mltbnz commented Apr 10, 2019

Did someone already create a custom map-style for a dark mode?

@lennet
Copy link
Member

lennet commented Apr 10, 2019

Did someone already create a custom map-style for a dark mode?

I think we have to wait until the sdk provides a public api for dark mode or a way to style maps locally.

The problem ist that custom styles for mapViews requires a webservices that provides the svg

@zutrinken
Copy link
Member

zutrinken commented Apr 10, 2019

Did someone already create a custom map-style for a dark mode?

@mltbnz excellent question!

@cbalster and I did some research on custom map tiles for Android based on OSM. The option is to host an own tile server, which might be quite expensive, or use a paid service. There are Mapbox and Thunderforest. The second could be an option in the future - they look quite open source friendly. Maybe there can be some cooperation, but I haven't reached out yet. That might be a job for @stephanlindauer with a better understanding of our traffic and approximately needed data package.

@mltbnz
Copy link
Member Author

mltbnz commented Apr 11, 2019

Seems like it is possible with a MKTileOverlay that points to a tile server. Would it be an option to use a already finished dark theme tile?

@mltbnz
Copy link
Member Author

mltbnz commented Apr 11, 2019

Found one here https://wiki.openstreetmap.org/wiki/Tile_servers but it doesn't look really nice I think
Screenshot 2019-04-11 at 13 14 35

@normansander
Copy link
Collaborator

Very cool @mltbnz . Can you please add it also to the CHANGELOG.md? https://github.com/criticalmaps/criticalmaps-ios/blob/master/CHANGELOG.md#added

https://keepachangelog.com/en/1.0.0/

Thanks :)

@mltbnz mltbnz added new feature and removed wip work in progress labels Apr 23, 2019
Copy link
Member

@zutrinken zutrinken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the dark map tiles, I think we could go with this first approach. It looks quite dark on my desktop screen, but at night on a phone and full brightness it might work quite good.

But can we have a darker highlight color here please:

Bildschirmfoto 2019-04-25 um 22 35 16

Chat/Twitter have a wired looking navigationbar:

Bildschirmfoto 2019-04-25 um 22 36 24

@mltbnz
Copy link
Member Author

mltbnz commented Apr 26, 2019

Great you took a look at the draft. The cell hightlightColor is one of the things on my list. Also changing the textColor of the overlay. I will also make the navigationBar non transparent. Another thing is the color of the chatBubble that is not handled and also I think I will create a seperate cell for each switch cell in settings since sometimes it seems to loose the reference to its selector I guess. Didn’t had the time to look into it properly. Other then that I am pretty happy with the result. The very dark map works kind of good.
The PR is touching a lot of code though. Makes the PR kind of unreviewable but I was not able to slice it into smaller chunks. There will be no tests though for the color changes since I would need to add UI Tests to the project.

@mltbnz mltbnz marked this pull request as ready for review April 29, 2019 12:55
@mltbnz mltbnz changed the title Dark mode Night mode Apr 29, 2019
.init(representation: NavigationOverlayItem.Representation(button: kniggeButton),
action: .navigation(viewController: getRulesViewController)),
.init(representation: NavigationOverlayItem.Representation(button: settingsButton),
action: .navigation(viewController: getSettingsViewController)),
])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated these to be buttons also for accessability

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes applying a theme easier since it is only one kind. For using the app with voice over it also makes sense. Plus shortens code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make a difference for VoiceOver and Theming because both solution are using UIButtons in the end. I don't like that we know have UIButton instances in the AppController

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to PR to roll back to the previous solution

@mltbnz
Copy link
Member Author

mltbnz commented Apr 29, 2019

There is still an issue with the night mode switch that sometimes is not switching the theme... Working on that. But the rest can be reviewed

@mltbnz
Copy link
Member Author

mltbnz commented Apr 30, 2019

settings switch cell issue should be fixed

Copy link
Member

@zutrinken zutrinken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here my findings:

Text should be white

Bildschirmfoto 2019-04-29 um 21 52 38

You can't scroll to the end in settings. The app info always scrolls back out of viewport

Bildschirmfoto 2019-04-29 um 21 53 13

turning night mode off didn't work so well here

Bildschirmfoto 2019-04-29 um 21 54 48

segmented control should be part of the header, means darker background color

Bildschirmfoto 2019-04-29 um 21 54 26

@mltbnz
Copy link
Member Author

mltbnz commented Apr 30, 2019

So for top 1: Seems out of my reach since it is a 3rd party. Could not manipulate it so far.
top 2: Did you check out the latest code? Think I fixed it this morning :)
top 3 and 4 are fixed. Thanks

@mltbnz mltbnz requested a review from zutrinken May 1, 2019 05:47
Copy link
Member

@lennet lennet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! The general implementation looks good to me, however there is still some work to do before we can merge it

CriticalMass/ThemeStore.swift Outdated Show resolved Hide resolved
CriticalMass/Colors.swift Show resolved Hide resolved
CriticalMass/ThemeDefining.swift Outdated Show resolved Hide resolved
CriticalMass/ThemeStore.swift Show resolved Hide resolved
CriticalMass/SettingsViewController.swift Outdated Show resolved Hide resolved
CriticalMass/MapViewController.swift Outdated Show resolved Hide resolved
CriticalMass/NibProviding.swift Show resolved Hide resolved
CriticalMass/TextFieldWithInsets.swift Show resolved Hide resolved
CriticalMass/UIWindow+Appearance.swift Outdated Show resolved Hide resolved
CriticalMass/UIWindow+Appearance.swift Show resolved Hide resolved
Copy link
Member

@zutrinken zutrinken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tabbar is missing its translucency and shadow.

Master Night mode
Bildschirmfoto 2019-05-02 um 11 28 23 Bildschirmfoto 2019-05-02 um 11 24 05

Segmented control in day mode has wrong navbar colour. text color is off as well.

Master Night mode
Bildschirmfoto 2019-05-02 um 11 36 00 Bildschirmfoto 2019-05-02 um 11 23 25

@mltbnz mltbnz requested review from zutrinken and lennet May 3, 2019 07:20
@mltbnz mltbnz requested a review from lennet May 7, 2019 07:10
@zutrinken
Copy link
Member

one last thing: The notification bubble number has to be white - in dark and light mode

Bildschirmfoto 2019-05-07 um 14 36 03

@mltbnz
Copy link
Member Author

mltbnz commented May 7, 2019

That must have slipped back somewhere. I fixed it and added two unit tests for safety 👍

Copy link
Member

@lennet lennet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@mltbnz mltbnz merged commit 9c13187 into criticalmaps:master May 8, 2019
@mltbnz
Copy link
Member Author

mltbnz commented May 8, 2019

💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants