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

feat: add username modes #4835

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

Conversation

snowfudge
Copy link

This adds username modes on both the chat message and the user list for to allow css styling. Users can then style names based on the username modes which only worked on the user list prior to this commit.

Currently it only exist on the userlist, e.g:

<div class="user-mode op">
  <span class="user" data-name="thelounge01" role="button">@thelounge01</span>
  <span class="user" data-name="thelounge02" role="button">@thelounge02</span>
</div>

<div class="user-mode half-op">
  <span class="user" data-name="halfop01" role="button">%halfop01</span>
  <span class="user" data-name="halfop02" role="button">%halfop02</span>
  <span class="user" data-name="halfop03" role="button">%halfop03</span>
</div>

But not the chat message, e.g.:

<div id="msg-2015949" class="msg" data-type="message" data-from="randomuser">
  <span
    aria-hidden="true"
    aria-label="7 February 2024, 14:07:27"
    class="time tooltipped tooltipped-e"
    >14:07</span
  >
  <span class="from">
    <span class="only-copy" aria-hidden="true">&lt;</span>
    <span class="only-copy" aria-hidden="true">&gt;&nbsp;</span>
  </span>
  <span class="user" data-name="randomuser" role="button">randomuser</span>
  <span class="content" dir="auto">Not working</span>
</div>

This PR aims to add user mode status on the username element so it'll show something like this both on the user list and chat message list:

<span class="user op" data-name="op01" role="button">@op01</span>
<span class="user half-op" data-name="halfop01" role="button">@halfop01</span>
<span class="user voice" data-name="voice01" role="button">@voice01</span>

This adds username modes on both the chat message and the user list for to allow css styling.
Copy link
Member

@brunnre8 brunnre8 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I do have some comments, see below

client/components/Username.vue Outdated Show resolved Hide resolved
client/components/Username.vue Outdated Show resolved Hide resolved
@snowfudge
Copy link
Author

Kindly let me know if there are any further changes I have to make to make this happen 😄

@brunnre8
Copy link
Member

rebase the PR and fold the fixup commit into the main one.

But no, I'm simply lacking time due to life.
Sorry for that, I'll get to it soon, promised.

@@ -1,6 +1,7 @@
<template>
<span
:class="['user', {[nickColor]: store.state.settings.coloredNicks}, {active: active}]"
:class="['user ', {[nickColor]: store.state.settings.coloredNicks}, {active: active}]"
Copy link
Member

Choose a reason for hiding this comment

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

why did you add a space here?

Copy link
Author

Choose a reason for hiding this comment

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

This was a goof-up on my side, apparently I didn't remove it when I moved the user modes to data-mode attribute. Will fix!

Copy link
Member

@brunnre8 brunnre8 left a comment

Choose a reason for hiding this comment

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

The one thing which is a bit funny now is that if a "mode" user speaks, they get the mode data-attribute set as expected.

However, if that user gets mentioned it does not.

Is this desired?

@@ -72,7 +74,10 @@ export default defineComponent({

const store = useStore();

const modeName = constants.modeCharToName;
Copy link
Member

Choose a reason for hiding this comment

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

modeName seems a poor name, why not keep it as modeCharToName?
It's a mapping, not a mode name.

Copy link
Author

Choose a reason for hiding this comment

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

That's fair, I'll have it changed

"@": "op",
"%": "half-op",
"+": "voice",
"": "normal",
Copy link
Member

Choose a reason for hiding this comment

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

You are dropping normal from the list, did you check that this is safe to do and it is in fact never hit?

Copy link
Author

Choose a reason for hiding this comment

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

My mistake - normal was used on the css as a grouping name so I placed it back.
https://github.com/snowfudge/thelounge/blob/c426de9e7e18ad0393b1356575e2b0701321158f/client/css/style.css#L1809-L1811

I've also added back the "!" based on PR#354 because I don't want to break things.
Changed operator -> op as well because again, it's used on the default styling as a grouping name

https://github.com/snowfudge/thelounge/blob/c426de9e7e18ad0393b1356575e2b0701321158f/client/css/style.css#L1797-L1799

@brunnre8 brunnre8 added the Status: changes-requested Review happened but some changes need to be made label Feb 19, 2024
@snowfudge
Copy link
Author

The one thing which is a bit funny now is that if a "mode" user speaks, they get the mode data-attribute set as expected.

However, if that user gets mentioned it does not.

Is this desired?

This is intended, yes. Initial reason was to style the username when they say something in the server so it's easier to, for example, know when an admin or an op says something. If they ping us, we get notified anyway by default. If they ping let say voiced users, said users will get the notification, which is why I deem it unnecessary to add the data atribute user mode to any pinged users of the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: changes-requested Review happened but some changes need to be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants