patterncssModerate
Cardshifter game lobby
Viewed 0 times
cardshiftergamelobby
Problem
We've been hard at work creating a browser-based GUI/Client for playing the Cardshifter TCG for the past week or so. Today, I just finished doing a pretty complicated layout for the chat lobby, along with @SirPython who put together the JavaScript/AngularJS code, which I will include with his permission.
Here's the full repo on GitHub, in case you're interested.
I have not worked with table-based HTML/CSS layouts for a long time, and I wanted to know if the code was well-structured, if I'm using CSS and Bootstrap efficiently, etc.
Here is
Here is
```
/ WHOLE LOBBY /
#lobby {
width: 100%;
}
/ TABLE HEADERS /
#lobby-headers {
font-family: Georgia, Times, "Times New Roman", serif;
text-a
Here's the full repo on GitHub, in case you're interested.
I have not worked with table-based HTML/CSS layouts for a long time, and I wanted to know if the code was well-structured, if I'm using CSS and Bootstrap efficiently, etc.
Here is
lobby.html. Note that the sections of text {{in brackets}} are injected by Angular. The header and other things are in a separate "mother" HTML file, where this child page (and others) get injected with the flow of the game.
Cardshifter Lobby
Deck Builder
Game invite from {{invite.name}} to play {{invite.type}}!
Messages
Users Online
[{{message.timestamp}}] {{message.from}}: {{message.message}}
{{user.name}}
Select game type:
{{mod}}
Here is
lobby.css which is linked to it. Note there are a number of empty selector classes, which all exist but some are not being used at the moment, and may be in the future if needed.```
/ WHOLE LOBBY /
#lobby {
width: 100%;
}
/ TABLE HEADERS /
#lobby-headers {
font-family: Georgia, Times, "Times New Roman", serif;
text-a
Solution
Well, there's lots of code needing review. I've done my best to split it into each language. I've avoided reviewing Angular, since it's a library I'm not comfortable with.
HTML:
-
You have tons and tons of
That is invalid HTML. You must use
According to the documentation,
-
Still using `
HTML:
-
You have tons and tons of
ng-* properties.That is invalid HTML. You must use
data-ng-*.According to the documentation,
data-ng-* is valid and will be detected.-
Still using `
and ? Are we in the 90s?
Those attributes are deprecated since HTML 4.01, use CSS instead.
Also, 400px is an invalid value. You have to specify the value without px (Yes, it only supports pixels and percentages).
Please, use CSS.
-
You have sparse fields everywhere!
There is not even a single !
Oh, wait! There is this: , but it doesn't even have a submit button or anything.
-
Some ids need some re-wording.
One example is id="lobby-chat-text-area". I know it is a textarea! What is it for? To make toasts? No! It's a message! For this example, changing to id="lobby-chat-message" would be better.
CSS:
-
You have empty styles!
For example: #lobby-deck-builder {}.
If they are empty, it means they aren't doing anything. If they aren't doing anything, you can remove them.
-
You have wacky indentations.
Look at this mess:
#lobby-message-list {
font-size: 0.8em; !important
}
/* List of all messages */
#lobby-chat-messages {
list-style-type: none;
padding-left: 0;
}
/* Each individual message line */
#lobby-chat-message {
}
While it makes it easier for you, it complicates stuff for others. Please, don't do this!
-
Repeated styles.
I'm sure you know what DRY means.
You have the following lines:
#lobby-message-list-header {
text-align: center;
}
#lobby-users-list-header {
text-align: center;
}
When, you could just write this:
#lobby-users-list-header, #lobby-message-list-header {
text-align: center;
}
Or move them into a class. The same applies for font-family.
JavaScript:
There isn't much to be said about your JavaScript.
-
You repeated var ENTER_KEY = 13; twice.
-
You are misusing objects.
You have the following:
var commandMap = {
"userstatus": updateUserList,
"chat": addChatMessage,
"inviteRequest": displayInvite,
"availableMods": displayMods,
"newgame": enterNewGame
};
Why don't you just attribute the functions here? Objects are good for that too!
-
You lost something on your function, updateUserList.
You have left a break; behind. The break there is quitting the for loop. You can simply use return;`.Code Snippets
#lobby-message-list {
font-size: 0.8em; !important
}
/* List of all messages */
#lobby-chat-messages {
list-style-type: none;
padding-left: 0;
}
/* Each individual message line */
#lobby-chat-message {
}#lobby-message-list-header {
text-align: center;
}
#lobby-users-list-header {
text-align: center;
}#lobby-users-list-header, #lobby-message-list-header {
text-align: center;
}var commandMap = {
"userstatus": updateUserList,
"chat": addChatMessage,
"inviteRequest": displayInvite,
"availableMods": displayMods,
"newgame": enterNewGame
};Context
StackExchange Code Review Q#100377, answer score: 14
Revisions (0)
No revisions yet.