criticalmaps / criticalmaps-ios

Critical Maps iOS App 🚲✊
http://www.criticalmaps.net
MIT License
287 stars 41 forks source link

minor layout fixes #369

Closed besilva closed 3 years ago

besilva commented 3 years ago

📝 Docs

📲 What

Fix some layout issues found by @mltbnz

🤔 Why

Our chat input was different from layout and acessory item color was hard to sse

👀 See

Screenshots, external resources?

Before 🐛 After 🦋
98852934-b5c15380-2458-11eb-96f3-6511da6fd349 1 IMG_2003 1
98852839-94606780-2458-11eb-9d09-6f8cd9144dd2 1 IMG_2004 1

♿️ Accessibility

besilva commented 3 years ago

364 Couldn't reproduce the problem with the event marker here :(

besilva commented 3 years ago

@mltbnz about the layout changes on the chat input, do you think the button should stick to the top when the textView grows? I can change to make it stick to the middle or the bottom

mltbnz commented 3 years ago

@besilva probably it makes sense to lay out the button on the bottom like in e.g. messages does. Wdyt?

image

besilva commented 3 years ago

@mltbnz makes sense to me. Similar approach to native UI makes easier for the user also in the bottom option it's near from the keyboard so it is easier to reach the button. Just made the update: Simulator Screen Shot - iPhone 12 - 2020-11-15 at 03 14 28

mltbnz commented 3 years ago

Cool. Also that the button now is inside the view 👍

besilva commented 3 years ago

I followed the design from the design repo. Thanks a lot for the approve and hope I can contribute more @mltbnz

besilva commented 3 years ago

Another thing is the changelog, should I create another version there or use the last one created?

mltbnz commented 3 years ago

Another thing is the changelog, should I create another version there or use the last one created?

I think it's best to start a new version there with a headline ## [Unreleased] the project refers to https://keepachangelog.com/en/1.0.0/ where you also find examples.

fbernutz commented 3 years ago

Oh didn't see the discussion here :) I've added a new Unreleased section on master, so just add it there. 🙏

fbernutz commented 3 years ago

The changes look good for me, but I'm still wondering why the snapshot test for AppSettingsVC didn't caught the tint color bug. Anyone an idea?

mltbnz commented 3 years ago

The changes look good for me, but I'm still wondering why the snapshot test for AppSettingsVC didn't caught the tint color bug. Anyone an idea?

I was also wondering 💭 but maybe the change is too subtle and is within the acceptable error margin 🤷‍♂️

fbernutz commented 3 years ago

The changes look good for me, but I'm still wondering why the snapshot test for AppSettingsVC didn't caught the tint color bug. Anyone an idea?

I was also wondering 💭 but maybe the change is too subtle and is within the acceptable error margin 🤷‍♂️

Mh, I've changed the precision from 99% to 100% again but the bug is still not caught. 🤔 Maybe it has something to do with the MockThemeController logic which is used in the snapshot test? -- edit: but I can't reproduce the bug with the missing tint color on the settings on the appstore version as well 🤔

mltbnz commented 3 years ago

The changes look good for me, but I'm still wondering why the snapshot test for AppSettingsVC didn't caught the tint color bug. Anyone an idea?

I was also wondering 💭 but maybe the change is too subtle and is within the acceptable error margin 🤷‍♂️

Mh, I've changed the precision from 99% to 100% again but the bug is still not caught. 🤔 Maybe it has something to do with the MockThemeController logic which is used in the snapshot test?

-- edit: but I can't reproduce the bug with the missing tint color on the settings on the appstore version as well 🤔

Now I also can't. Either something is messed up or I got a really buggy version that crashes and presents me visual errors. because it can 🤷‍♂️😁

besilva commented 3 years ago

Another thing is the changelog, should I create another version there or use the last one created?

I think it's best to start a new version there with a headline ## [Unreleased] the project refers to https://keepachangelog.com/en/1.0.0/ where you also find examples.

I was wondering if you guys would create a new Unreleased or I should do it. That's why i asked here

besilva commented 3 years ago

The changes look good for me, but I'm still wondering why the snapshot test for AppSettingsVC didn't caught the tint color bug. Anyone an idea?

I was also wondering 💭 but maybe the change is too subtle and is within the acceptable error margin 🤷‍♂️

Mh, I've changed the precision from 99% to 100% again but the bug is still not caught. 🤔 Maybe it has something to do with the MockThemeController logic which is used in the snapshot test? -- edit: but I can't reproduce the bug with the missing tint color on the settings on the appstore version as well 🤔

Now I also can't. Either something is messed up or I got a really buggy version that crashes and presents me visual errors. because it can 🤷‍♂️😁

This is a guess: Maybe it is a iOS 14.1 bug, because I could see the problem in both simulator and my device but I did not look which version of iOS we use in the CI

fbernutz commented 3 years ago

The changes look good for me, but I'm still wondering why the snapshot test for AppSettingsVC didn't caught the tint color bug. Anyone an idea?

I was also wondering 💭 but maybe the change is too subtle and is within the acceptable error margin 🤷‍♂️

Mh, I've changed the precision from 99% to 100% again but the bug is still not caught. 🤔 Maybe it has something to do with the MockThemeController logic which is used in the snapshot test? -- edit: but I can't reproduce the bug with the missing tint color on the settings on the appstore version as well 🤔

Now I also can't. Either something is messed up or I got a really buggy version that crashes and presents me visual errors. because it can 🤷‍♂️😁

This is a guess: Maybe it is a iOS 14.1 bug, because I could see the problem in both simulator and my device but I did not look which version of iOS we use in the CI

Interesting, we're currently still using Xcode 12.0 in the CI, so the tests run on iOS 14.0. (https://github.com/criticalmaps/criticalmaps-ios/blob/master/.github/workflows/main.yml#L20) I can't reproduce the tint color bug in the simulator with iOS 14.1 and 14.2, also not on my own device with iOS 14.2.

But I think we can merge this nevertheless. ✌️

mltbnz commented 3 years ago

When setting the theme to light, then kill the app and open again I was able to reproduce it. If you then switch to system or dark theme and back to light the tint is fine. Not that big of a deal for now

fbernutz commented 3 years ago

When setting the theme to light, then kill the app and open again I was able to reproduce it. If you then switch to system or dark theme and back to light the tint is fine. Not that big of a deal for now

Interesting... even with that instruction, I'm not able to reproduce the bug in the current AppStore version.

mltbnz commented 3 years ago

When setting the theme to light, then kill the app and open again I was able to reproduce it. If you then switch to system or dark theme and back to light the tint is fine. Not that big of a deal for now

Interesting... even with that instruction, I'm not able to reproduce the bug in the current AppStore version.

Ok. Getting too crazy for me 😛 Let's merge then and next time ship your version 😁