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

Improved whisper menu #227

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

Mitchdev
Copy link
Contributor

Show all your whispers and indicate if the user is online

image

@Mitchdev Mitchdev added the enhancement New feature or request label Apr 18, 2023
@Mitchdev Mitchdev requested a review from 11k April 18, 2023 06:29
Copy link
Contributor

@11k 11k left a 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?

@Mitchdev
Copy link
Contributor Author

For some reason the prettier commit didn't merge when I merged the master branch in.

Copy link
Contributor

@11k 11k left a 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.

@Mitchdev Mitchdev requested a review from 11k April 20, 2024 11:39
Comment on lines +100 to +102
<span class="time" title="${time.format(
'LLL',
)}">${time.fromNow()}</span>
Copy link
Contributor

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.

Comment on lines +1384 to +1385
-moz-border-radius: 7.5px;
-webkit-border-radius: 7.5px;
Copy link
Contributor

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 {
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants