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.

Sign up for GitHub

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

Jump to bottom

moved logs from drawer menu to the settings #1552

Merged
merged 3 commits into from
May 18, 2016
Merged

moved logs from drawer menu to the settings #1552

merged 3 commits into from
May 18, 2016

Conversation

AndyScherzinger
Copy link
Contributor

As discussed in #1432 the Logs menu item from the drawer has been moved to the settings screen.

@jancborchardt I moved it to the "more" section instead of the "advanced" section - is this correct or should I move it to the "advanced section?
device-2016-03-22-145222

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @davivel, @tobiasKaminsky and @jabarros to be potential reviewers

This was referenced Mar 22, 2016
Closed
Closed
@AndyScherzinger
Copy link
Contributor Author

@jancborchardt I can also move the settings item in the drawer to the bottom, or separate it with a line like ather apps do it. Whatever floats your boat 😉

@AndyScherzinger
Copy link
Contributor Author

Google apps just separate it with a line:
Gmail
device-2016-03-22-150703
Drive
device-2016-03-22-150808
Hangout
device-2016-03-22-150738

Dropbox - top right
device-2016-03-22-150846

Amaze file manager like Google bat as a sticky footer
device-2016-03-22-150908

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt having done a local implementation using the Support Library for the drawer's content implementation the menu items would look like this...
device-2016-03-22-200828
with an item "active":
device-2016-03-22-200848
The difference is very, very subtile since the unactive grey is quite close to the primary color (dark blue).
The Drawer width has also increased to our actual one since this is the standard with in material...

What do you think? Moving on with this has it's benefits since checked/unchecked state, items via native menu.xml files/groups would then be available :)

@AndyScherzinger AndyScherzinger mentioned this pull request Mar 22, 2016
Closed
@jancborchardt
Copy link
Member

I'd say either we stick it to the bottom or we leave it as it is. The line just increases the focus on settings more, which is not what we want. ;)

@AndyScherzinger
Copy link
Contributor Author

Okay :9 So when the reliable uploads are in place (on master) with the new drawer it would then look like this:
device-2016-03-22-224635

@AndyScherzinger
Copy link
Contributor Author

I removed the new drawer implementation here and started a new PR for the drawer over here: #1559
So this PR is basically a move of the logs to the settings screen and a removal from the drawer so you can forget about the latest screenshots since this won't be part of this PR anymore.

@AndyScherzinger AndyScherzinger self-assigned this Mar 24, 2016
@AndyScherzinger
Copy link
Contributor Author

@rperezb @jancborchardt @davivel @tobiasKaminsky This is DONE and can be reviewed...

@tobiasKaminsky
Copy link
Contributor

👍 code is fine!

@AndyScherzinger AndyScherzinger force-pushed the 1432_logs_to_settings_screen branch 2 times, most recently from 55fbfba to 9c2c936 Compare April 14, 2016 07:46
@LukeOwlclaw LukeOwlclaw mentioned this pull request Apr 14, 2016
Closed
@AndyScherzinger AndyScherzinger force-pushed the 1432_logs_to_settings_screen branch 3 times, most recently from 249ae77 to ae69363 Compare April 26, 2016 20:48
@AndyScherzinger
Copy link
Contributor Author

AndyScherzinger commented May 3, 2016 •
edited
Loading

pinging @davivel and @malkomich -> minor change, freshly rebased, community reviewed, resolving #1432 raised by @jancborchardt

@rperezb
Copy link

rperezb commented May 6, 2016

@jancborchardt @AndyScherzinger we will check this after the team week thx

@AndyScherzinger AndyScherzinger force-pushed the 1432_logs_to_settings_screen branch 2 times, most recently from c1500d9 to 56e7fdb Compare May 9, 2016 11:24
@jabarros
Copy link
Contributor

👍

davivel
davivel reviewed May 11, 2016
View reviewed changes
res/values/setup.xml
@@ -53,6 +53,7 @@
<bool name="imprint_enabled">false</bool>
<bool name="recommend_enabled">true</bool>
<bool name="feedback_enabled">true</bool>
<bool name="logger_enabled">true</bool>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the same reason the other are there ;) --> with this flag you can decide if the log part will be rendered in the settings screen or not. We can take it out if you want to. I just thought like with imprint, feedback etc. you would like to be able to have to feature toggle for logging also since I discussed the move to the settings screen with @jancborchardt and we agreed that after moving it to the settings screen it will also be visible in the official release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I understand now. I totally oversaw the intention of making the menu available in the official release of the app, not only in debug mode. In this case, having this option here makes a lot of sense.

The point is, I'm not so sure we should have this is the official app. I will follow this up in #1432

@davivel
Copy link
Contributor

davivel commented May 11, 2016

👎 ; please, see my question

@AndyScherzinger
Copy link
Contributor Author

😉 please see my answer 😃

@davivel
Copy link
Contributor

davivel commented May 11, 2016

Thanks, @AndyScherzinger .

Blocked until a decision is taken about make the logs available in release mode or not.

cc @rperezb

@jancborchardt
Copy link
Member

Ok – let’s make it available in debug mode / beta only maybe for now. And just move the place of it. We’re doing 2 separate steps here (moving, and making it visible for normal users), that’s the problem.

@davivel
Copy link
Contributor

davivel commented May 12, 2016

Thanks, @jancborchardt.

Proposal: let's keep the option in settings.xml, but with default value to false. Besides, let's modify the condition in Preferences.java so that the option is available if (loggerEnabled || BuildConfig.DEBUG). Seems to me more comfortable.

@AndyScherzinger , what do you think?

@AndyScherzinger
Copy link
Contributor Author

Yeah, sounds good to me! I can make the change during the next days! :)

@AndyScherzinger
Copy link
Contributor Author

@davivel added the changes as discussed :)

@davivel
Copy link
Contributor

davivel commented May 16, 2016

Great job :)

👍

@jesmrec , your turn.

@jesmrec
Copy link
Collaborator

jesmrec commented May 18, 2016

@davivel @AndyScherzinger @jabarros

Approved 👍

@jabarros
Copy link
Contributor

Cool, so it is prepared to be merged :)
@AndyScherzinger could you rebase it on master and merge it by yourself? Thanks!!

@AndyScherzinger AndyScherzinger added this to the 2.0.1-current milestone May 18, 2016
@AndyScherzinger AndyScherzinger merged commit 1f0fa0a into master May 18, 2016
@AndyScherzinger
Copy link
Contributor Author

DONE! 👍

@AndyScherzinger AndyScherzinger deleted the 1432_logs_to_settings_screen branch May 18, 2016 10:43
@rperezb
Copy link

rperezb commented May 23, 2016

Thanks!
Good to know that there is a new variable on the setup file ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@AndyScherzinger AndyScherzinger

Labels
Approved by qa Design
Projects
None yet
Milestone
2.0.1-current
Development

Successfully merging this pull request may close these issues.
8 participants
@AndyScherzinger @mention-bot @jancborchardt @tobiasKaminsky @rperezb @jabarros @davivel @jesmrec