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

Desktop: Fix selectors for notification button #3209

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

mjangda
Copy link
Member

@mjangda mjangda commented Feb 10, 2016

This is a hotfix for the notifications shortcut in the desktop app to catch up with the new masterbar updates.

To Test

  • Checkout Desktop repo
  • Checkout this branch in calypso submodule
  • make run
  • Verify that Notifications from menu and shortcut (cmd + 4) work

@mjangda mjangda added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. labels Feb 10, 2016
this.onToggleNotifications();
clearNotificationBar: function() {
// TODO: find a better way. seems to be react component state based
let notificationsLink = this.getNotificationLinkElement();
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor, but when we have an identifier that we don't reassign to (notificationsLink, in this case), we generally use const.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch; updated!

This fixes the notifications shortcut in the desktop app to catch up with the new masterbar updates.
@mjangda mjangda force-pushed the fix/desktop-notif-shortcut branch from 49b635c to 378bda4 Compare February 10, 2016 21:17
mjangda added a commit that referenced this pull request Feb 10, 2016
Desktop: Fix selectors for notification button
@mjangda mjangda merged commit 23b9855 into master Feb 10, 2016
@mjangda mjangda deleted the fix/desktop-notif-shortcut branch February 10, 2016 21:30
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants