Skip to content

Implement menu and functionality to discard tabs, fixes #11.#33

Open
sfleiter wants to merge 1 commit intoautonome:masterfrom
sfleiter:add-menu
Open

Implement menu and functionality to discard tabs, fixes #11.#33
sfleiter wants to merge 1 commit intoautonome:masterfrom
sfleiter:add-menu

Conversation

@sfleiter
Copy link

Merry Christmas!

This pull request adds a tab menu for Dormancy to discard selected, left, right or other tabs.
If anything comes up during review please do not hesitate to tell me about it.

@sfleiter
Copy link
Author

Any feedback on this @autonome?

Copy link
Owner

@autonome autonome left a comment

Choose a reason for hiding this comment

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

Thanks very much for the PR, and thanks for poking me about it!

I've left a few comments on the PR, mostly minor.

One question is about the use of "retire". I'm not sure it's the right word here. I've been using asleep/awake, so that there's no worry of tabs actually going away. Even those aren't absolutely consistent with "dormant", but I think they're closer?

// Load (or reload) config from storage
let oldConfig = config;
config = await loadConfig();
addMenu();
Copy link
Owner

Choose a reason for hiding this comment

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

minor nit: add a space between this and the config loading bit, with a comment explaining what's happening

Copy link
Author

Choose a reason for hiding this comment

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

will do

"description": "Menu item to retire all selected / highlighted tabs."
},
"menuDiscardLeft": {
"message": "Retire tabs to the &left",
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I'm not familiar enough with localizing entities - what happens with "&left"?

Copy link
Author

Choose a reason for hiding this comment

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

That l should be underlined and should be an access key.
Have to test this myself on windows as mac does not support this.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it works on Windows in my VM.
I did on purpose create one for the menu group since that could collide with future Firefox tab menu entries.

See here (access keys are underlined):
virtualbox_windows 10-klon_29_01_2019_22_23_09

@sfleiter
Copy link
Author

One question is about the use of "retire". I'm not sure it's the right word here. I've been using asleep/awake, so that there's no worry of tabs actually going away. Even those aren't absolutely consistent with "dormant", but I think they're closer?

I thought a bit more about it and would like to use "put so sleep" throughout according to https://www.merriam-webster.com/dictionary/put%20to%20sleep bullet 3.
That would move "do not sleep tabs that play sound" to "do not put tabs to sleep that play sound".
I just saw that you used that term already so I assume that should be okay.

Maybe it is time for more consistency and use Dormany only as the extension name and otherwise stay with (put to) sleep and awake or wake up?

@sfleiter
Copy link
Author

New version of the code pushed

@sfleiter sfleiter force-pushed the add-menu branch 2 times, most recently from f42645c to 1bc7794 Compare January 29, 2019 23:53
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.

2 participants