search

ICT4Dat / ict4dat-news-android

ICT4D.at's App to combine all ICT4D news into one Android application
Apache License 2.0
5 stars 1 forks source link

Refs #125 - Add Setting Screen #148

Closed rajasone closed 5 years ago

rajasone commented 5 years ago

ict4d-settings

spipau commented 5 years ago

Quick question, why do you have 2 settings fragments? SettingsFragment and SettingsFragmentWithToolbar - is it not possible to just use the SettingsFragment?

I didn't try it myself, I'm just wondering why? (I guess you had a reason 😉 )

rajasone commented 5 years ago

@spipau As our current application architecture each fragment is responsible to show/handle it's toolbar, SettingsFragment is of type PreferenceScreen so it is not possible to include a toolbar in SettingsFragment, BaseFragment is responsible to handle the toolbar and Up Navigation so i created a fragment named as SettingsFragmentWithToolbar which is just a container for Toolbar and SettingsFragment, I think it is a clean solution to re-sue the BaseFragment toolbar and navigation handling

spipau commented 5 years ago

Maybe we can improve this condition because currently if i open my app at 11:55 pm and after 5 minutes i open my app again (12:00am) this condition return true because we have different day of the month, it is working fine because of the another condition which checks the size of existing news, may be we can use Difference class to get the difference in hours, i think it will be more efficient, it is not something really critical it's just a thought :) please check this link for Difference class, thanks :)

You're right, but I don't think that it is a big problem. More like a nice to have thing, I don't think we need to be very precise here. But good point 😉

spipau commented 5 years ago

@spipau As our current application architecture each fragment is responsible to show/handle it's toolbar, SettingsFragment is of type PreferenceScreen so it is not possible to include a toolbar in SettingsFragment, BaseFragment is responsible to handle the toolbar and Up Navigation so i created a fragment named as SettingsFragmentWithToolbar which is just a container for Toolbar and SettingsFragment, I think it is a clean solution to re-sue the BaseFragment toolbar and navigation handling

OK, I thought so, thanks, I guess it's OK 😄

spipau commented 5 years ago

Should we really offer the Sync News After feature? I don't know if this necessary. Best practice is to always keep settings to a minimum.

rajasone commented 5 years ago

@spipau i don't know either, you are right, may be we just allow user to Allow Sync Service option only

spipau commented 5 years ago

@rajasone Thanks for removing the option. I just went through your changes and you implemented some logic to display the date of the last update, two things:

  1. It doesn't work, I have after a first install news items in my list, but in the settings it displays Never
  2. Honestly, I would also remove this setting, because it doesn't bring any benefit to the user, but just makes our application more complex. I understand why you put it in, we, as developers are interested in these information, but every normal user is simply not. They don't care when the last updates was, as long the next one will happen. A good best practice for settings is, to keep it really to the minimum and only, really only put in a setting option if it is really necessary.