-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add compress message option for users #49
Conversation
private onMessageContentClicked = (id: string, index: number) => { | ||
const { markAsDecompressed, logs } = this.props | ||
|
||
const message = logs[index] | ||
|
||
if (isMessage(message) && message.compressed) { | ||
markAsDecompressed(id) | ||
|
||
// Clear the cache to force a height rerender if necessary | ||
this.logMeasureCache.clear(index, 0) | ||
} | ||
} | ||
|
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.
Not sure why this new method exists instead of re-using onClickMessage()
?
@@ -193,6 +208,9 @@ export class Logs extends React.Component<Props> { | |||
actionHandler, | |||
addHighlightsIgnoredUser, | |||
alternateMessageBackgrounds, | |||
addUserToCompress, |
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.
Bit of a weird name imo, maybe addUserToCompressList
or compressUser
@@ -193,6 +208,9 @@ export class Logs extends React.Component<Props> { | |||
actionHandler, | |||
addHighlightsIgnoredUser, | |||
alternateMessageBackgrounds, | |||
addUserToCompress, | |||
deleteUserFromCompress, |
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.
Maybe removeUserFromCompressList
or uncompressUser
@@ -217,22 +235,28 @@ export class Logs extends React.Component<Props> { | |||
const chatter = _.get(chatters, log.user.id) | |||
const isBanned = _.isNil(chatter) ? false : chatter.banned | |||
const useAlternate = alternateMessageBackgrounds && index % 2 === 0 | |||
const isCompressedUser = compressedUserIds.indexOf(log.user.id) >= 0 |
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.
Definitely not cheap (O(n)), we should use a structure where we can have direct access (maybe a POJO?)
|
||
LogComponent = ( | ||
<Message | ||
messageIndex={index} |
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.
Not really a message index, more of a logIndex
|
||
/** | ||
* Text when message should be compressed | ||
*/ | ||
CompressedTxt: "....." |
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.
Discussed in the PR, but this should not exist and at least use …
@@ -506,6 +507,13 @@ export class ChatClient extends React.Component<Props, State> { | |||
return | |||
} | |||
|
|||
const userId = parsedMessage.user.id; | |||
const indexOfCompressedUser = this.props.compressedIds.indexOf(userId); |
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.
As before, cost too much, we should go with a direct access.
if (indexOfCompressedUser > -1) { | ||
serializedMessage.compressed = true | ||
} |
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.
The serialized message should not be modified here but in libs/Message
instead by padding new parsing options if needed.
@@ -71,6 +72,8 @@ export default class Message implements Serializable<SerializedMessage> { | |||
|
|||
this.badges = this.parseBadges(userstate, parseOptions.hideVIPBadge) | |||
this.message = this.parseMessage(message, userstate, currentUsername) | |||
|
|||
this.compressed = false |
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.
Logic should go here instead of mutating this after serialization.
@@ -75,6 +77,7 @@ export const initialState = { | |||
alternateMessageBackgrounds: false, | |||
autoFocusInput: true, | |||
autoHostThreshold: 1, | |||
compressedUserIds: [], |
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.
This should use another data structure, maybe a POJO
Thanks for the PR 😄 On top of the code review, here are my more general comments: 1. DecompressNot sure if it's just me or not, but decompress feels weird. When quickly googling for it, found this website mentioning decompress is to relieve the pressure or compression on something and uncompress is to restore a compressed file to its normal size. Kinda feels like we should go for uncompress. I didn't point out all the occurrences in the code review (around 16 occurrences). 2. MechanismI don't feel like all messages should be compressed even if the user is someone in the "compressed list". For example, in this screenshot, only the second message which spans over 2 lines should be compressed. Regarding the compression itself, I don't feel like all the content of the message should be replaced with dots. We should only do an ellipsis on the cutting point using the In the previous screenshot, if we go with only the
An interesting issue with this approach tho is how to handle properly the responsiveness when someone is for example resizing its window involving messages that should (or are) compressed. 3. CursorWhen having your cursor on a compressed message, we should use a different type of cursor to indicate the click action to uncompress the message (totally missed that before checking the code). We should also add a tooltip (an alt may be enough tho) with something like Click to uncompress - followed either by a preview of the compressed message or the entire message maybe. 4. SettingsThis feature needs its own settings otherwise you can't use it (add/remove people) without a message from said users. Altho, the current UI for the settings is totally full and we have no room to add a new settings page. I've started a redesign of the settings which should add more room for various settings sections. When I have the time to finish this, we should update this PR to use it and add proper settings dedicated to this feature. (blocked until new settings UI is finished) 5. Context menuI feel like the context menu entry should be between Regarding the name of the menu entries, feels a bit long, what about Compress messages and Uncompress messages? 6. Instant feedbackEnabling compression on a message doesn't compress the previous messages so there is no feedback to the action. We should compress previous messages from the user. Same goes for uncompressing of course. 7. LintingAs mentioned in the code review, looks like there is something wrong with Prettier & the linting process in this PR, I'll fix this so we can later update this PR and re-run the linting process on it. |
Thank you for the very clear explanation of what can be improved for this PR. Once I have some time I will start on processing your requests and will update the PR accordingly. Feature designThis feature was based on my own design meaning that there was no input from other designers perspective on how the feature should work. So thank you for describing your design wishes. Maybe an setting would be nice on how the message should be compressed, a couple of options would be to compress the full message which is the current implementation. A second option would be your requested implementation where you can see the first part of the message until a new line is hit and if new ideas arise for new options those ideas can be added to this list as well. Settings menuMy thought was also to put this in the settings menu but since the settings menu was full I instead put the option to toggle the compress/uncompress feature in the message itself. I did see this as a point for improvement which I totally agree with you on that. |
That's also a possibility if you're up for it altho I think the default option should be the one I mentioned.
The menu addition is great, we do that already for things that have their own settings section, but it's only a bonus as it requires a message to use it.
Definitely leave it, it's neat 👍
Don't bother creating a custom thing, we'll add once the settings when there is more room in the settings modal and ship it properly. I should have more time this time of the year to work on this project so the new UI for the settings shouldn't take long to finish. |
The master branch has been updated and now contains a fix that should highlight the linting issues in the PR that I mentioned in the code review which were not failing earlier. |
The master branch has also been updated to contain a new UI for the settings as discussed earlier. There is an empty slot for the new panel needed for this PR and will also make it easier in the future to add more panels. Not sure yet where the new panel section will fit, its title & icon but that's something we can tweak later on, even after this PR is merged. |
Just saw that GitHub auto-closed this PR when I renamed the Feel free to re-open if you ever come back to this. |
Describe the pull request
I was getting tired of the responses from bots taking so much space in the chat rooms. So I decided that I wanted to add an option to YaTA which allowed the user to compress messages from certain users. If the user wants to see the full message they can click the compressed message and the message is expanded to its original size.
To activate the compression press the hamburger menu and click
enable message compression
. If the feature is active for the selected user the buttondisable message compression
will be shown instead.Why
Because I felt like adding this feature to YaTA. Trying to improve perfection 👼
How
Some new actions have been added which change the state of the message. A new message property has been added to see if this message currently is compressed. Some changes were made to the database to keep track of which user ids the message needs to be compressed by default.
Also a big shoutout to RomaniaHate on Twitch for helping me with the React/Redux stuff.
Screenshots
Possible future improvements