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

Add help view for keyboard shortcuts #91

Merged
merged 10 commits into from Jun 20, 2017
Merged

Add help view for keyboard shortcuts #91

merged 10 commits into from Jun 20, 2017

Conversation

Gomez
Copy link
Member

@Gomez Gomez commented Sep 20, 2016

No description provided.

@@ -0,0 +1,18 @@
<tbody>
<tr>
<th>Keyboard shortcut</th>
Copy link
Member

Choose a reason for hiding this comment

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

l10n? 😉

@@ -5,6 +5,8 @@
class="button new-button"
href="{{addAccountUrl}}">{{ t 'Add mail account' }}</a>

<p><a id="keyboard_shortcuts"
href="{{keyboardShortcutUrl}}">{{ t 'Keyboard shortcuts' }}</a></p>
Copy link
Member

Choose a reason for hiding this comment

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

closing tag should be in the next line

@@ -159,6 +161,9 @@ define(function(require) {
}));
}
},
showKeyboardShortcuts: function() {
this.content.show(new KeyboardShortcutView({}));
Copy link
Member

Choose a reason for hiding this comment

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

please also store which view is active (take a look at the other methods)

};
},
regions: {
accountsList: '#settings-accounts'
},
events: {
'click #new_mail_account': 'addAccount'
'click #new_mail_account': 'addAccount',
'click #keyboard_shortcuts': 'showKeyboardShortcuts'
Copy link
Member

Choose a reason for hiding this comment

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

no underscores please

@ChristophWurst ChristophWurst added this to the 0.6.1 milestone Sep 20, 2016
@Gomez Gomez force-pushed the shortcut_help branch 2 times, most recently from 61bf63b to 2f12543 Compare October 20, 2016 11:15
@ChristophWurst ChristophWurst modified the milestones: 0.6.2, 0.6.3 Dec 8, 2016
@Gomez
Copy link
Member Author

Gomez commented Mar 28, 2017

All tests pass, ready for review @nextcloud/mail

@@ -36,14 +36,6 @@ define(['views/settings', 'views/helper'], function(SettingsView) {
it('produces the correct HTML', function () {
settingsview.render();

html = settingsview.el.innerHTML.trim();
Copy link
Member

Choose a reason for hiding this comment

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

Interesting approach to fix the failing tests 😜

Copy link
Member Author

Choose a reason for hiding this comment

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

:) I know. HTML comparison is a pain. Lets check for the link text only. Should be good enough? Or?

@jancborchardt
Copy link
Member

Working on the design atm :)

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks very good! I just found some nitpicks :)


});
});
});
Copy link
Member

Choose a reason for hiding this comment

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

new line missing

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member

Added a commit to improve the design, please also review core nextcloud/server#4183 (needs backports also)

@jancborchardt
Copy link
Member

capture du 2017-03-30 12-51-15

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member

Fixed/added proper URL routing support.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 for @Gomez's part. Nicely done, thanks :)

@ChristophWurst
Copy link
Member

@nextcloud/mail please give this a quick test. This is ready to be integrated into master IMO.

@ChristophWurst
Copy link
Member

The major changes are from @Gomez which I've approved -> merging this now 🚀

@ChristophWurst ChristophWurst merged commit 3397e01 into master Jun 20, 2017
@ChristophWurst ChristophWurst deleted the shortcut_help branch June 20, 2017 08:51
@ChristophWurst ChristophWurst moved this from TO REVIEW (max 4) to DONE in Christoph's Tasks Jun 20, 2017
@ChristophWurst ChristophWurst modified the milestones: 0.6.5, 0.7 Aug 2, 2017
@lock
Copy link

lock bot commented Jan 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and questions.

@lock lock bot locked and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants