-
Notifications
You must be signed in to change notification settings - Fork 60
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
Improved whisper menu #227
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to finally take a look at this. 😅 Would you mind merging in master and fixing any merge conflicts?
For some reason the prettier commit didn't merge when I merged the master branch in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind fixing the merge conflicts, and humanizing the date next to each conversation (e.g., "a month ago")? You can give the date a title so that it displays a local time in a localized format on hover.
<span class="time" title="${time.format( | ||
'LLL', | ||
)}">${time.fromNow()}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind using code tags to make this a little more readable? It's a dependency we added sometime after this PR.
-moz-border-radius: 7.5px; | ||
-webkit-border-radius: 7.5px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are vendor prefixes still necessary here?
color: $color-accent-light; | ||
.badge { | ||
color: $color-chat-text1; | ||
.section { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this CSS challenging to read and I think it's a combination of there being no newlines before each nested ruleset and excessive nesting. For instance, you don't need this level of specificity to select .unread
.
#chat-whisper-users .section .user-entry .right .unread {
}
I think the issue is you're trying to make the nesting in your CSS reflect the nesting in your markup. Besides readability, you can also run into situations where you have to rely on !important
more often to override styles.
Show all your whispers and indicate if the user is online