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.

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

Cycle though history with keyboard #5

Merged
merged 12 commits into from Nov 12, 2014
Merged

Cycle though history with keyboard #5

merged 12 commits into from Nov 12, 2014

Conversation

odrevet
Copy link
Contributor

@odrevet odrevet commented Nov 11, 2014

In this revision Alt modifiers has been added to the schema and extension.js has the correct indentation so it shows what has been added compare to your version.

@Tudmotu
Copy link
Owner

Tudmotu commented Nov 12, 2014

Hey @odrevet,
Thank you for this feature, I would really like to merge this, but there are a few issues I see with the code:

  • Please fix your indentation so to align the keys you've added (_bindShortcut, etc..) with the other keys of the object (_removeEntry, Name, Extends, etc..)
  • Do not use var in Gnome Shell extensions. Replace all the var instances with let
  • In lines 331, 352, you are using 'for-in' on an array. Use Array.forEach. Take a look at line 170 for an example.
  • I mentioned it in the previous PR - Please add an option to modify the shortcuts. We do not want users to be bound by our decision. They might have custom shortcuts that collide, so they need to be able to change these shortcuts. A nice bonus would be to also add an option to completely disable the shortcuts.

@Tudmotu
Copy link
Owner

Tudmotu commented Nov 12, 2014

@odrevet - Awesome! :)
If you like, what we can do regarding the last issue (expose shortcuts in the settings), is merge the changes so far, and open a different ticket for exposing the shortcuts via settings, then you can work on that part separately.
What do you say?

@odrevet
Copy link
Contributor Author

odrevet commented Nov 12, 2014

Ok let's do that !

2014-11-12 17:41 GMT+01:00 Yotam Bar-On notifications@github.com:

@odrevet https://github.com/odrevet - Awesome! :)
If you like, what we can do regarding the last issue (expose shortcuts in
the settings), is merge the changes so far, and open a different ticket for
exposing the shortcuts via settings, then you can work on that part
separately.
What do you say?


Reply to this email directly or view it on GitHub
#5 (comment)
.

@Tudmotu
Copy link
Owner

Tudmotu commented Nov 12, 2014

@odrevet - OK, I apologize, but two last things before I merge:

  • If I restart Gnome-Shell, there is a black box that appears on the top left side of the screen. Can you look into that and see if you can fix it, so it won't appear?
  • Please add these lines to the CSS file (for better readability of the popup):
font-size:2em;
padding:.5em;

@odrevet
Copy link
Contributor Author

odrevet commented Nov 12, 2014

done. I also set a fixed width in the css

2014-11-12 18:07 GMT+01:00 Yotam Bar-On notifications@github.com:

@odrevet https://github.com/odrevet - OK, I apologize, but two last
things before I merge:

  • If I restart Gnome-Shell, there is a black box that appears on the
    top left side of the screen. Can you look into that and see if you can fix
    it, so it won't appear?
  • Please add these lines to the CSS file (for better readability of
    the popup):

font-size:2em;padding:.5em;


Reply to this email directly or view it on GitHub
#5 (comment)
.

Tudmotu added a commit that referenced this pull request Nov 12, 2014
Cycle through history with keyboard
@Tudmotu Tudmotu merged commit d4d0302 into Tudmotu:master Nov 12, 2014
@Tudmotu
Copy link
Owner

Tudmotu commented Nov 12, 2014

Thank you @odrevet :)
Great feature!

@odrevet
Copy link
Contributor Author

odrevet commented Nov 12, 2014

thanks to you. Hope the folks on gnome.org will like it too !

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.

None yet

2 participants