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

Batch of crash fixes and tweaks #21

Merged
merged 50 commits into from
Mar 15, 2017

Conversation

rock3r and others added 30 commits November 24, 2016 21:42
See upcoming blog post™
Use exclusions for IDE settings ignores
@rock3r
Copy link
Author

rock3r commented Jan 14, 2017

There is a lot of changes I'm afraid, had to do a pass of reformatting of the app module to have uniform code style.

@TarasKunyk
Copy link
Member

Hello, thanks for your update. We have a few notes/questions on the current source base:

  1. There is database crash on the migration procedure
  2. Timezone dialog is shown each time details screen is dismissed
  3. You have added social media activity instead of the fragment. What's the point of such update?
  4. You have hardcoded toolbar height in the resources. Wouldn't ?android:attr/actionBarSize constant from the compatibility library better suit this situation?
  5. social_query is defined with build properties instead of being loaded from server. This limits app flexibility a bit. don't you think so?

@rock3r
Copy link
Author

rock3r commented Jan 16, 2017

@TarasKunyk here's the answers

  1. Please create an issue on the fork so I can look into it (there is no "previous" version of this and we didn't change the DB, so I'm not sure what you're migrating from)
  2. Ditto, please create an issue
  3. I'm trying to gradually move the app to the one-activity-per screen model, which makes a thousand times easier to deal with lifecycle and callbacks. For example, with that model it's almost impossible to have the broken behaviour we currently have with multiwindow on Android N.
  4. That's a private resource, you should not be using it directly (the IDE will even warn you about that) as it might change/go away at any time
  5. I've never seen the need for that, if you find yourselves doing that all the time we can look into that later on (but again, never seen a conference changing their social hashtag anyway, so it's probably YAGNI)

@rostyslawbulych
Copy link
Member

Hi @rock3r! Seems that your update will result in a significant app update. And it will take some time to make it stable. So what do you think about moving changes to separate branch named connfa_v2 and go on working with it. While previous app version will be published on connfa_v1 and master branches since it's currently stable. What do you think?

@rock3r
Copy link
Author

rock3r commented Jan 18, 2017

Hi @rostyslawbulych! Sure, works for me. If you create the branch (branching off of github-master) I can change the target for this PR and rebase all our future work against that.

For more details on what we're planning to do, please take a look at the issues and boards on our fork. Feel free to get in touch if you have questions/suggestions, you should have my details.

@TarasKunyk
Copy link
Member

  1. You are right. That's some old bug, I had pushed fix to the repo
    2.-
  2. Sidebar pattern does require fragments usage for the best user experience. Do you have any alternatives?
  3. Anyway, that value isn't constant. It's different in landscape mode and for tablets. So we have to declare all values possible. And it's defined within compatibility library so it isn't OS-dependend and safe to use.
  4. You are right, that value probably will never be updated. But sometimes it isn't defined at the moment of application release (on iOS app release takes almost a week) so we do need this feature and would like t keep both app versions as close as possible in terms of functionality.

@rock3r
Copy link
Author

rock3r commented Jan 23, 2017

Hey there @TarasKunyk!

  1. 👍
  2. 👍
  3. The navigation drawer pattern can be achieved with a seamless UX when using one-activity-per-page too; all apps I have worked on so far have the nav drawer and don't use fragments. It's all a matter of doing overridePendingTransition() at the right place
  4. You're right about needing different values but that only applies if you explicitly support landscape (we currently don't) and tablet-specific layouts (we don't either). If/when we manage to put those in then we'll need to add the overloads in values-land and values-sw600dp, but we don't need them yet. Actually the fact the support lib does that regardless of us not supporting either case is a problem as we have 64 dp toolbars on tablets but the rest of the layout is a phone layout. I double checked and it's not a private resource anymore in the latest support library version, so at least that should be ok now, but I'd still not do it for the aforementioned issues.
  5. Ok, what if we had as a feature to implement to try and fetch that data from the backend and rely on the built-in value (as implemented here) if there's none? Can make sure it's in the next bundle PR.

PS I'm changing the base of this PR to the v2 branch

@rock3r rock3r changed the base branch from github-master to connfa_v2_dev January 23, 2017 15:39
@TarasKunyk
Copy link
Member

  1. Such workaround won't allow us to reproduce same user experience as fragments provide: Screen animation will be a bit different from the original one. Plus you won't be able to add hamburger menu button to the subscreens and handle it correctly. We would like to keep existing sidebar functionality.
    But could you provide us a few samples of your apps using your navigation approach?

Additionally I would like to note that we could apply sidebar navigation pattern to activity-based app using deprecated ActivityGroup API. Sample project is attached to the post.
NestedActivity.zip

@rock3r
Copy link
Author

rock3r commented Jan 30, 2017

Well, the functionality can be preserved. That's the whole point of me proposing it, if it weren't possible I wouldn't.

As for the ActivityGroup, it seems a rather dangerous path to go down. At that point I'd rather keep the fragments.

@TarasKunyk TarasKunyk merged commit 630a2ff into lemberg:connfa_v2_dev Mar 15, 2017
@rock3r rock3r deleted the Address_crashes branch April 1, 2017 08:27
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.

6 participants