1203014 - Make Log Out a button (visually)

Status: RESOLVED FIXED

(Firefox for iOS :: Theme & Visual Design, defect)

Description

[Robin Andersen [:tecgirl]]

Attachment: Settings-2.png

We're beginning to create a pattern with direct action buttons (see bug 1201642) and should break out the "Log Out" button from the About list. See attached for an example.

Comment 1

[Pikor]

Attachment: Pull request

Comment 2

[Stephan Leroux [:sleroux]]

Comment on attachment 8687576 [details] [review]
Pull request

Hey Pikor - thanks for the patch! Looks good - just noticed a bug where the section separator lines still appear when not logged in (left a comment with a picture on the PR).

Comment 3

[Stephan Leroux [:sleroux]]

Hey Pikor - are you still interested in patching up the PR with the comments I left?

Comment 4

[Pikor]

Hi Stephan,
yes, I'm definitely interested in patching this PR, even more, taking another one. Sorry for such long delay, I need to better organise my time for this project.

Comment 5

[Pikor]

Update: https://github.com/pikor/firefox-ios/commit/7906907a7bda44534f1587755dc46bdf0a8ce8e8

Comment 6

[Stephan Leroux [:sleroux]]

Comment on attachment 8687576 [details] [review]
Pull request

Looks good! Thanks for revisiting this. Before I can merge it you'll need to rebase the branch. Also, it would be nice if both commits were merged into a single commit. You can check out http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html if you're unfamiliar with squashing commits. Make sure to also include the Bug number/description in the commit like you did for the first commit as well.

Comment 7

[Pikor]

Stephan,

first of all, thanks for review. Secondly thanks for helpful link to git squash explanation. I followed article step-by-step and I ended in situation:
* squashing was successfully performed (https://github.com/pikor/firefox-ios/commit/30442a68859808c71092a07a98f68a9bc6397687)
* I had to pull, (then auto-merge happened) in order to be allowed to push. Which is weird since no changes where made, and now there is basically empty commit with 0 changes (https://github.com/pikor/firefox-ios/commit/838f0d12b7fbe672d9b16f4514d4009940c29614).

I did this process twice (with hard-reset to previous commit) and both times output was: do the merge.
I did squashing for the first time. I feel like this merge commit shouldn't be there, please advise on next steps.

Comment 8

[Stephan Leroux [:sleroux]]

One of the side effects of rebasing/squashing the commits is that the makes your branchs history not match with the origin server (Github in this case). Probably when you tried to push the squashed branch it rejected it? The only way around this is to do a force-push (git push origin <branch-name> --force) to override the origin's view of the branch. Unfortunately there isn't another way (that I know of) to do this without using --force. As a note of caution, using --force may have undesired side effects since it will essentially override anything the remote server has so double check that what you want to push is indeed what you want.

Comment 9

[Pikor]

Attachment: git log 13.01.2016.png

Comment 10

[Pikor]

Correct me if I'm wrong, what you are saying is: I should do force-push after squashing in order to avoid merge?
In terms of current situation: everything is pushed and repo on my Mac is up-to-date with github repo (see attachment)

Comment 11

[Stephan Leroux [:sleroux]]

Hm looks like your commit history became wonky after the merge. Instead of merging from the mozilla repo, you should rebase.

Here's something we can try to fix up the history:

Cleaning up your branch history
1. Run git log --graph --abbrev-commit --decorate --date=relative. This will pretty print a nice graph of your branches.
2. Find the commit from which you branched off of master
3. Run 'git reset --soft <commit here>' This will reset you back to when you branched off of master with all your changes you made in a 'staged' state. 
4. From here you can add the changes you want and create a brand new, single commit.

Syncing up your fork

There are some good instructions here: https://help.github.com/articles/syncing-a-fork/. This will update your repos code with the latest changes from the Mozilla repo.

Rebasing your branch

1. Now with an up-to-date version of master, checkout your feature branch.
2. Rebase your feature branch from master to update by calling 'git rebase master'

At this point you should see your commit on top of all the changes on master. After confirming this, go ahead and 'git push origin <branch-name> --force' to update your origin/PR.

Comment 12

[Pikor]

Stephan,
first of all, thank you for detailed, step-by-step instructions. It was much easier for me to perform fix. It wasn't easy, since there were lot of changes since my latest push, but now there is only 1 commit on my branch and it should work as expected. 
Please review latest changes.

Also, slightly off-topic: is it possible to develop new features of Firefox? I didn't find that information in Contributor guidelines

Comment 13

[Stephan Leroux [:sleroux]]

Hey Pikor,

Thanks for updating the PR with the latest changes. I realized this morning that between the time this patch was written and rebased I landed a pretty large refactoring of the way we do settings and it seems that your rebase got caught in the storm :(
I left a comment on the PR about some changes that we don't need because of the refactor but after that everything _should_ be good to go :)

> Also, slightly off-topic: is it possible to develop new features of Firefox? I didn't find that information in Contributor guidelines

Definitely! Contributors can participate in feature development as well. Pretty much any feature we are planning on working on should have a bug filed in Bugzilla. Was there something in particular you had in mind that you wanted to work on? I know filtering through Bugzilla can be challenging to find a feature to work on but we can come up with a list to help give some direction if thats something you're interested in.

Comment 14

[Pikor]

>I realized this morning that between the time this patch was written and rebased I landed a pretty large refactoring of the way we do settings and it seems that your rebase got caught in the storm :(

No problem with that. Software has to evolve, I waited too long with fixing this bug, I'll fix what's left according to your comments.

In terms of features.
In general, before fixing bug 1203014 I thought it would be cool to work on Firefox iOS project but not only to fix bugs but also to develop new features. So, in order to keep balance, I thought of switching between bug-fixing/new features every one task.
And I tried to find some list of current tasks/features roadmap but I end up asking you question because I found nothing.

And yes I had something particular in mind. I thought of adding 3D touch* support to Firefox. More and more apps gained this support in recent weeks. Also Apple is willing to promote apps that implements this kind of "shiny new things" (like Enhanced with 3D Touch category in the App Store). Not sure if they promote Firefox, which is somehow competitor for Safari, but user-experience would be also improved.
Also, Today widget might be handy for users, but not sure what to put there. This screen can be accessed without unlocking phone so we need to take into account privacy issues.

Let me know what you think. I'll try to fix 1203014 this weekend.

* Shortcuts example:
1. Favourites
2. Sync
3. Reading list

P.S
>Contributors can participate in feature development as well. Pretty much any feature we are planning on working on should have a bug filed in Bugzilla.
If I understand it correctly new features are marked as bugs?

Comment 15

[Pikor]

Whoa! I hope we are good to go. What I did:
1. Applied changes according to your comment on github to `SettingsTableViewController`
2. Commit those changes
3. Squashed two commits into one
4. git push --force (and your comment is no longer visible on github. No wonder - history was changed, but If I didn't use force I would end up in situation like here https://bugzilla.mozilla.org/show_bug.cgi?id=1203014#c7

Is everything okay now?

Comment 16

[Stephan Leroux [:sleroux]]

Hey Pikor,

Thanks for updating the patch. Everything looks good now!

> And yes I had something particular in mind. I thought of adding 3D touch* support to Firefox.

We actually just finished implementing 3D touch on master and will be in the upcoming 2.0 release. Sorry for the lack of transparency behind the roadmap. 

> Also Apple is willing to promote apps that implements this kind of "shiny new things" (like Enhanced with 3D Touch category in the App Store)

This has been something we want to focus on for the upcoming releases. Along with 3D Touch we also implemented Spotlight integration so you'll be able to see search results for open tabs in Spotlight for 2.0. Another feature I think we're looking at for the next release is doing something with the Today widget but I think James might have picked up that bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1186573.

> If I understand it correctly new features are marked as bugs?

Yup. Usually larger features might be prefixed with [meta] as a way to encapsulate a group of related 'bugs' for a feature. For example: https://bugzilla.mozilla.org/showdependencytree.cgi?id=1198418&hide_resolved=1

Comment 17

[Stephan Leroux [:sleroux]]

master 44cc3064c334ac65dce03af7097a0457fd3c0b65