Skip to content
This repository has been archived by the owner on Mar 20, 2019. It is now read-only.

#9 Add confirmation when closing a group #34

Closed
wants to merge 5 commits into from
Closed

#9 Add confirmation when closing a group #34

wants to merge 5 commits into from

Conversation

xiio
Copy link
Contributor

@xiio xiio commented Apr 2, 2016

i've added confirmation prompt on group closing. When group is empty there is no need to confirm.

denschub
denschub reviewed Apr 3, 2016
View reviewed changes
src/lib/utils.js
@@ -1,5 +1,6 @@
const _ = require("sdk/l10n").get;
const PrefService = require("sdk/preferences/service");
const chrome = require("chrome");
Copy link
Owner

Choose a reason for hiding this comment

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

const {Cc, Ci} = require("chrome");

@denschub denschub added this to the 0.5.0 milestone Apr 3, 2016
@xiio
Copy link
Contributor Author

xiio commented Apr 3, 2016

I've fixed code according to your notes.

denschub
denschub reviewed Apr 3, 2016
View reviewed changes
src/lib/tabmanager.js
@@ -206,6 +206,10 @@ TabManager.prototype = {
return recentlyAddedGroup;
},

getGroupTabCount: function(tabBrowser, groupID) {
return this._storage.getTabIndexesByGroup(tabBrowser, groupID);
Copy link
Owner

Choose a reason for hiding this comment

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

You missed the .length.

@denschub
Copy link
Owner

denschub commented Apr 3, 2016

Okay, I've tested it. It... works, but the dialog box is somehwat annoying to me since I usually have a lot of temporary groups. I'd suggest adding a preference to turn off the question.

Preferences can be added in package.json and you can use Prefs.prefs.[name] in index.js to get their value. Please add the title of your preference to en-US.properties, although you'll have to repeat the same string twice. This is to make translating the stuff a bit easier. Use [name]_title (and [name]_description if you want to add a longer description) as the key in the upper block of the language file and the SDK will do its magic.

After that, let's squash your commits into a single one. In larger PRs, multimple commits may be okay but commits lke "added missing .length" just bloat up the git history. To squash, use git rebase -i develop and replace every pick but the first through squash. Git will then ask you for a new commit message and I'd suggest to use something like Add confirmation when closing a group. After that, you have to force-push your branch, but that's okay!

@xiio
Copy link
Contributor Author

xiio commented Apr 5, 2016

how can i add new icon? i mean "undo" icon.
I see it is Font Awesome, but it is customized set...

@denschub
Copy link
Owner

denschub commented Apr 5, 2016

I've added the undo-icon in bebc291. Rebase and you can use .fa-undo

@xiio xiio force-pushed the group_close_confirm branch from fa5f6df to 1a599d1 Compare April 10, 2016 20:06
@xiio
Copy link
Contributor Author

xiio commented Apr 10, 2016

I've changed the form of this confirmation. Now it doesn't need so much interacition from user. I've added new pref for this feature so it is configurable. Check this out.

@xiio
Copy link
Contributor Author

xiio commented Apr 25, 2016

Is there any problem?

denschub
denschub reviewed Apr 29, 2016
View reviewed changes
src/data/action_creators.js
@@ -4,5 +4,11 @@ const ActionCreators = {
type: "TABGROUPS_RECEIVE",
tabgroups: tabgroups
};
},
setGroupCloseTimeout: function(timeout) {
Copy link
Owner

Choose a reason for hiding this comment

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

A little newline here would be nice. :)

@denschub
Copy link
Owner

Sorry, I've been busy with stuff. I just added some remarks! Thanks for your work so far.

@denschub
Copy link
Owner

@xiio any chance you'll finish this?

@xiio
Copy link
Contributor Author

xiio commented May 25, 2016

Sorry, I've been busy lately. I will fix it till Tuesday (31.05)

@denschub
Copy link
Owner

Awesome! 😸

Don't hurry, just wanted to know whether I should take this over myself or not. :)

@xiio
denschub#9 Add confirmation when closing a group
fbcb721 
 - counter removed
 - default group delay sets to 3
 - lint fixes
@xiio
Copy link
Contributor Author

xiio commented Jun 8, 2016

I've made fixes according to your suggestions.

denschub
denschub reviewed Jun 9, 2016
View reviewed changes
Jakefile
@@ -17,14 +17,14 @@ task("cleanup", () => {

desc("runs wslint on the built source");
task("lint", ["build"], {async: true}, () => {
jake.exec([`cd ${DIST_DIR}; eslint .`], {
jake.exec([`cd ${DIST_DIR} & eslint .`], {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm sorry? This runs cd in the background while running eslint in another directory. What were your intentions here?

@denschub
Copy link
Owner

denschub commented Jun 9, 2016

Thanks, we're getting close! 👍 Sorry for being kinda picky, but I'd rather stick with a clean and nice code base or we might create a monster that nobody will ever be able to support.

@xiio
Copy link
Contributor Author

xiio commented Sep 17, 2016

All done. Please review...

@denschub denschub modified the milestones: 0.5.0, 0.6.0 Oct 25, 2016
@denschub denschub mentioned this pull request Oct 25, 2016
Open
@denschub
Copy link
Owner

Sorry for the insane delay, I got totally sidetracked. Merged as 3a869eb now, thank you!

@denschub denschub closed this Jan 15, 2017
@denschub denschub mentioned this pull request Jan 15, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@xiio @denschub