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

Reading user "do not disturb" status #60

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Reading user "do not disturb" status #60

merged 1 commit into from
Aug 15, 2018

Conversation

Mazuh
Copy link
Contributor

@Mazuh Mazuh commented Aug 11, 2018

Is there any icon that should be used for this "snooze"? That's how it looks currently:
screen shot 2018-08-11 at 19 21 36

@haskellcamargo
Copy link
Owner

Can you take a look on this, @duynguyenhoang?

@duynguyenhoang
Copy link
Collaborator

duynguyenhoang commented Aug 14, 2018

Let me check it

else:
return ''

def change_snooze_indicator(self, is_snoozed=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think is_snoozed=False, am I correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also change status indicator icon when is_snoozed is changed

Copy link
Contributor Author

@Mazuh Mazuh Aug 14, 2018

Choose a reason for hiding this comment

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

I think is_snoozed=False, am I correct?

I meant to set a snooze to True or False only if someone explicitly said so. It was just a (dumb) semantic idea. But yeah, it could be only is_snooze=False (and then I'd remove the if below)!

Also change status indicator icon when is_snoozed is changed

That led me to the other thread's question: should this snooze be a separated indicator or just be the same as presence_icon?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this indicator should be only one, but we have some statuses

  • Snooze
  • Online

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried to update it slightly, please check https://i.imgur.com/at0XuwE.png

if is_online:
presence_icon = ('presence_active', ' {} '.format(get_icon('online')))
self.presence_icon = ('presence_active', ' {} '.format(get_icon('online')))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, we should add snooze icon like:

if is_snoozed:
            self.presence_icon = ('presence_active', ' {} '.format(get_icon('snooze')))
        

I have checked https://nerdfonts.com/ and see we can use "snooze": "\uF9B1",

Copy link
Contributor Author

@Mazuh Mazuh Aug 14, 2018

Choose a reason for hiding this comment

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

so the presence_icon should indicate snooze too? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, it should be snooze icon.

Copy link
Contributor Author

@Mazuh Mazuh Aug 14, 2018

Choose a reason for hiding this comment

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

ok, then, I'll do it this evening!

@Mazuh
Copy link
Contributor Author

Mazuh commented Aug 15, 2018

@duynguyenhoang I updated the code applying your reviews. Also removed the (snoozed) string as it seems not needed anymore, and the display string for snooze is set as "presence_snooze". All good with that?

I have a problem, however. I don't know what triggered this, but my icon has a black background:
screen shot 2018-08-15 at 08 53 41

@property
def body(self):
if self.is_snoozed:
presence_icon = ('presence_snooze', ' {} '.format(get_icon('snooze')))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please change presence_snooze to presence_active, It is actually the color of the icon
So the icon becomes
snooze

Copy link
Contributor Author

@Mazuh Mazuh Aug 15, 2018

Choose a reason for hiding this comment

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

Aaaah, got it, thanks. will do.

Should this 'snoozed' string be there too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think yes, good to have, because not everyone use the icon so the text should be helpful

@Mazuh
Copy link
Contributor Author

Mazuh commented Aug 15, 2018

done! 👍

screen shot 2018-08-15 at 11 10 11

@Mazuh
Copy link
Contributor Author

Mazuh commented Aug 15, 2018

Also, I saw a Todo at #64 that's related to this snooze feature. I left the is_snoozed in the state object so you can use it there.

@duynguyenhoang
Copy link
Collaborator

Please rebase your branch, I am going to merge it then.
Thank you for your efffort.

@Mazuh
Copy link
Contributor Author

Mazuh commented Aug 15, 2018

@duynguyenhoang did it!

@duynguyenhoang
Copy link
Collaborator

I mean rebase your branch, merge your commits into one commit.
Like below command

git rebase -i HEAD~5

@Mazuh
Copy link
Contributor Author

Mazuh commented Aug 15, 2018

Oh, I thought you were going to "squash and merge" via GitHub, so I just used rebase to sync with master. Anyway, did it too.

@duynguyenhoang duynguyenhoang merged commit adb0023 into haskellcamargo:master Aug 15, 2018
@duynguyenhoang duynguyenhoang mentioned this pull request Aug 16, 2018
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.

3 participants