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

Timezone implementation #297

Merged
merged 17 commits into from
May 11, 2021
Merged

Timezone implementation #297

merged 17 commits into from
May 11, 2021

Conversation

andzejsw
Copy link
Contributor

Android implementation should now work with Timezone logic. So should example build.
Currently haven't looked on iOS.

@nickrandolph
Copy link
Contributor

@andzejsw did you want to fix up the ios build before we merge this PR in? Or did you want to create separate item to track the iOS work? My concern with merging this in without the iOS piece is that the code in develop would currently in an unusable state for iOS.

@andzejsw
Copy link
Contributor Author

@nickrandolph I would like to take a look on iOS soon. Hopefully will fix iOS part in this PR. I will let you know what is my progress.
Also feel free to test Android part.

@andzejsw andzejsw changed the title WIP: Timezone implementation Timezone implementation Apr 27, 2021
@andzejsw
Copy link
Contributor Author

@nickrandolph tested on iOS. Logic works also there. There was some errors in example app not related to this issue which I also fixed. In my opinion we can merge this PR to dev branch. But you and @thomassth also can test if you want before merging.
As far as I se, there is multiple issues regarding this PR, so I guess we need to publish this as soon as we can.

@andzejsw
Copy link
Contributor Author

Should I increase version!?

@andzejsw
Copy link
Contributor Author

andzejsw commented May 2, 2021

@thomassth, @nickrandolph Can anyone of you take a look on this PR? And changes I made?

@nickrandolph
Copy link
Contributor

Yep give the version a small increment (third digit is fine). I will try to review the changes in next couple of days

@andzejsw
Copy link
Contributor Author

andzejsw commented May 2, 2021

Thanks. Did it.

nickrandolph
nickrandolph previously approved these changes May 6, 2021
@nickrandolph
Copy link
Contributor

nickrandolph commented May 6, 2021

@andzejsw can you rebase your branch please - I've reviewed these changes but when I got to merge I'm getting an error stating that "This branch cannot be rebased due to conflicts"
Once this has been done, I should be able to merge these changes - thanks for the hard work on this

@andzejsw
Copy link
Contributor Author

andzejsw commented May 6, 2021

It should be ok now. I did git rebase.

@andzejsw
Copy link
Contributor Author

andzejsw commented May 7, 2021

@nickrandolph You can later check if changes can now be merged.

@andzejsw
Copy link
Contributor Author

andzejsw commented May 7, 2021

Latest commit fixes issue #241

nickrandolph
nickrandolph previously approved these changes May 9, 2021
@nickrandolph
Copy link
Contributor

nickrandolph commented May 9, 2021

@andzejsw I'm still seeing rebase issues due to conflicts. Did you rebase off develop or master? Also, make sure you rebase off the remote builttoroam:develop branch, rather than the local develop on your fork (unless you've updated your develop)

@thomassth
Copy link
Contributor

thomassth commented May 9, 2021

@andzejsw I'm still seeing rebase issues due to conflicts. Did you rebase off develop or master? Also, make sure you rebase off the remote builttoroam:develop branch, rather than the local develop on your fork (unless you've updated your develop)

I think when merging #287 , somehow a rebase was done instead of a merge. This causes all other cocurrent commit incompatible, and if we force merge it will look like this:

Original -> 1' -> 2' -> 3' -> 1 -> 2 -> 3 -> all new commits

And we have 8 commits on the chain.

I propose squashing the 8 commits in builttoroam:develop, then merge commits as normal. The tree should look cleaner and should not cause too much problem:

Original -> (1-3)' -> 1 -> 2 -> 3 -> all new commits

(This is related to merging, but may take some discussion to solve, so we may need to open an issue for longer discussions.)

@nickrandolph
Copy link
Contributor

nickrandolph commented May 9, 2021

Yes, we do a "rebase and merge" when merging in changes to develop - this leaves a cleaner commit history.
If you fetch builttoroam:develop and then rebase your local branch onto it, you should be good to go.
Ping me on Twitter (https://twitter.com/thenickrandolph) if you want to talk through this

@andzejsw
Copy link
Contributor Author

I will take a look to rebase with builttoroam:develop, but yeah, it looks like a lot of extra work. Will see how much time it takes.

@andzejsw
Copy link
Contributor Author

@nickrandolph rebasing done.

@thomassth thomassth mentioned this pull request May 10, 2021
1 task
@nickrandolph
Copy link
Contributor

@andzejsw did you force push after rebasing? you need to force push otherwise you end up with duplicates in the commit history - if you notice there's twice as many commits in the history than there were previously

@andzejsw
Copy link
Contributor Author

@nickrandolph there is nothing to push for me now from andzejsw/device_calendar branch. And all changes are pulled from builttoroam:develop.

@nickrandolph
Copy link
Contributor

Yep you'd need to do the rebate again but this time doing a force push. Sorry to be such a pain about this but it helps to keep the commit history clean.

@andzejsw
Copy link
Contributor Author

@nickrandolph hopefully it will be ok. Else I have no idea how it is done.

@andzejsw
Copy link
Contributor Author

Rebasing broke functionality. Will take a look what i can do to fix this.

@andzejsw
Copy link
Contributor Author

@nickrandolph Everything rebased and looks like, that now everything works as it should

@nickrandolph nickrandolph merged commit 952518d into builttoroam:develop May 11, 2021
@nickrandolph
Copy link
Contributor

@andzejsw a big thank you for all the time and effort put into getting this PR across the line!

@andzejsw
Copy link
Contributor Author

@nickrandolph, @thomassth I think that these changes should be published to pub.dev as Prerelease or even new release. What do you think?

@nickrandolph
Copy link
Contributor

@andzejsw do you mind pushing a quick PR with updated version number and updates to readme (if required). I'll merge the PR and then do a PR to release to push a new prerelease version

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

Successfully merging this pull request may close these issues.

3 participants