Conversation
b782894 to
f4a0b8d
Compare
|
@creesch I could use your help testing with someone leaving and joining a server. |
f4a0b8d to
269265d
Compare
269265d to
ad20743
Compare
GC will take care of this stuff
ca3a41c to
2347474
Compare
|
I'm not sure about adding this to the PR as it's a little out there, but I have also considered performing declarative DOM building like this: /**
* Creates a new player element.
* @param {StoredPlayerInfo} player
* @returns {HTMLElement}
*/
#createPlayerElement(player) {
// Create a new DOM element if none exists for the player.
const playerElement = document.createElement('li');
playerElement.setAttribute('role', 'listitem');
playerElement.setAttribute('data-player-id', player.playerId);
// Add click event to insert the player name into the chat input
playerElement.addEventListener('click', () => {
const cursorPos = this.#chatInput.selectionStart;
const textBefore = this.#chatInput.value.substring(0, cursorPos);
const textAfter = this.#chatInput.value.substring(cursorPos);
this.#chatInput.value = `${textBefore}${player.playerDisplayName}${textAfter}`;
this.#chatInput.focus();
this.#chatInput.selectionStart = this.#chatInput.selectionEnd =
cursorPos + player.playerDisplayName.length;
});
playerElement.appendChild((() => {
const headContainer = document.createElement('div');
headContainer.className = 'player-head-container';
headContainer.title = `${player.playerDisplayName}'s head`;
headContainer.appendChild((() => {
const headImg = document.createElement('img');
headImg.className = 'player-head';
headImg.src = player.playerTextureUrl;
headImg.setAttribute('aria-hidden', 'true');
headImg.onerror = () => {
headImg.src = '/img/steve.png';
};
return headImg;
})());
headContainer.appendChild((() => {
const headOverlay = document.createElement('img');
headOverlay.className = 'player-head-overlay';
headOverlay.src = player.playerTextureUrl;
headOverlay.setAttribute('aria-hidden', 'true');
headOverlay.onerror = () => {
headOverlay.src = '/img/steve.png';
};
return headOverlay;
})());
return headContainer;
})());
playerElement.appendChild((() => {
const nameSpan = document.createElement('span');
nameSpan.className = 'player-name';
nameSpan.textContent = player.playerDisplayName;
nameSpan.title = player.playerName;
// Specifically for aria labels show both playerDisplayName and playerName if they are different.
if (player.playerDisplayName !== player.playerName) {
nameSpan.setAttribute(
'aria-label',
`Display name: ${player.playerDisplayName}, Username: ${player.playerName}`,
);
} else {
nameSpan.setAttribute(
'aria-label',
`Player name: ${player.playerDisplayName}`,
);
}
return nameSpan;
})());
return playerElement;
} |
creesch
left a comment
There was a problem hiding this comment.
Overall it looks good to me, I still need to test it though. Will do that in a bit.
One thing I did notice is that the lock and requestAnimationFrame is gone. Which should be mostly fine, except for maybe bigger servers with a lot more people online. Something I did try to keep in mind when I initially wrote this.
Going to ponder that a little bit, but it is probably fine. Once I have had a look at it running I'll complete the review.
|
I'm curious about your use of
It doesn't seem to be the performance fix it was intended to be. Perhaps sensible if there was some kind of batching of the work - build X elements, add them to the DOM, defer to next frame and repeat. Edit: I am also suspicious of the similar use of |
|
Yeah, your assessment of So also the removal of the |
|
My understanding of |
|
All fair, looks good otherwise as I said. As far as this goes
I don't have very strong opinions about this. On the one hand building an element first makes sense but doing it in the append callback like in your example does encapsulate it nicely where it is being added. If you want to do DOM building in this way I don't have an objection to that. |
|
I'll hold off on the fancy DOM building for now. Honestly a lightweight component library might make more sense. |
|
Even a component library feels overkill for what this is. We don't have a ton of reusable components and the ones we have can easily be handled already. |
This release contains a variety of new features and improvements. Things like support for newer minecraft versions, tab complete of usernames, hover events and a bunch more! # Main ## New - Translations are now retrieved per chat message from the game at run time and stored with the message. (#84) Meaning that mod messages will display, messages will show in the language you will have set and that the mod now officially supports **1.21.1 and up** as messages will always have the correct translation for that minecraft version. - Support added for the `/tell`, `/msg`, `/w` and `/me` commands. (#82) Note: This mod is never going to support all commands as this easily can be used for abuse. - Click events are now supported (#86) - Tab completion of usernames (#78) - Use "Jetbrains Mono" font for chat message/input/player names making it look more like minecraft chat (#80) - Support for legacy color/format codes and multi-space formatting (#69) ## Fixes/enhancements - Make pingPatterns case insensitive (#63) - Added aria labels and use semantic elements (#58) - Hover text now renders similar to in game (#85) and supports color code (#87) # Technical - Restructured front-end code to not only be modules but classes (#59) - Simplify player heads rendering (#70) - A variety of dependencies has been updated. # Various other For a complete overview see the [commit history](https://github.com/creesch/minecraft-web-chat/commits/main/) # Screenshots 
When it was added I felt there was unnecessary JS complexity in handling player head rendering/updating.
<img>tags (should be optimal for performance)player_list.mjsOverall this removes more code than it adds.