HiveBrain v1.2.0
Get Started
← Back to all entries
patterncssModerate

Cardshifter game lobby

Submitted by: @import:stackexchange-codereview··
0
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 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 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.