DEV Community

Dahye Ji
Dahye Ji

Posted on • Updated on

Login page UI / code review

I asked for code review for the first time and I'd like to write about what I heard back.
Login UI

1) Don't write unnecessary code.

I didn't need div class="first-page-container" here. I thought I needed it for styling but I could've given the style to body instead since I already had flex property inside body. *I should try to write html efficiently.
This was my code below.

<body>
    <div class="first-page-container">
        <section class="login-first-page">
            <h1 class="sr-only">위니브 로그인 페이지</h1>
            <img class="logo-img" src="src/img/weniv.png" alt="logo">
            <p>위니브를 더 즐겁고 편리하게 이용하세요.</p>
            <button id="goto-login-btn" type="button" onclick="window.location.href='login.html'">로그인</button>
        </section>
    </div>
</body>
Enter fullscreen mode Exit fullscreen mode

This was my css

body {
  position: relative;
  box-sizing: border-box;
  display: flex;
  justify-content: center;
  font-family: "Spoqa Han Sans Neo", "sans-serif";
  background-color: #f2f2f2;
}

.first-page-container {
  display: flex;
  text-align: center;
  align-items: center;
  height: 100vh;
}
Enter fullscreen mode Exit fullscreen mode

I removed div class="first-page-container" from my html, removed .first-page-container and added this two lines to the body instead.

text-align: center;
align-items: center;
Enter fullscreen mode Exit fullscreen mode

2) Give clear information when writing img's alt value. 

<img class="logo-img" src="src/img/weniv.png" alt="logo">
Enter fullscreen mode Exit fullscreen mode

I only wrote it as logo but it might be helpful if it explains about the image clearly like "weniv company logo" for example.

3) Write clean code! 

<body>
    <div class="first-page-container">
        <section class="login-first-page">
            <h1 class="sr-only">위니브 로그인 페이지</h1>
            <img class="logo-img" src="src/img/weniv.png" alt="company logo">
            <p>위니브를 더 즐겁고 편리하게 이용하세요.</p>
            <button id="goto-login-btn" type="button" onclick="window.location.href='login.html'">로그인</button>
        </section>
    </div>
</body>
Enter fullscreen mode Exit fullscreen mode

I forgot to remove id="goto-login-btn" because I was going to make function using the id but instead I ended up putting onclick inline but forgot to remove id that I never used.

4) If button doesn't have width or height, screen reader wouldn't recognise the button and wouldn't read it. 

 <button class="close-btn">
  <img class="close-page-icon" src="src/img/close.png" alt="로그인 창닫기 아이콘">
</button>
Enter fullscreen mode Exit fullscreen mode 
main.login-container button.close-btn {
  position: absolute;
  border: none;
  background: transparent;
  height: 0;
  top: 18px;
  right: 24px;
}

main.login-container button.close-btn img.close-page-icon {
  top: 0;
  left: 0;
  width: 16px;
  height: 16px;
}
Enter fullscreen mode Exit fullscreen mode

I wrote the height:0 because I wanted to put replacement image instead of normal button being there and I thought I had to hide the button but my image is actually inside the button tag😅 So giving height to button doesn't make difference. It doesn't display the ugly default button style anyways since I gave these properties & values below.

border: none;
background: transparent;
Enter fullscreen mode Exit fullscreen mode

So, I gave height and width to button. Also, I forgot to add button type and had to add type="button" as well.

<button type="button" class="close-btn">
  <img class="close-page-icon" src="src/img/close.png" alt="로그인 창닫기 아이콘">
</button>
Enter fullscreen mode Exit fullscreen mode 
main.login-container button.close-btn {
  position: absolute;
  border: none;
  background: transparent;
  top: 11px;
  right: 17px;
  width: 30px;
  height: 30px;
}
main.login-container button.close-btn img.close-page-icon {
  width: 16px;
}
Enter fullscreen mode Exit fullscreen mode

5) Don't forget to write text inside <label> tag which explains the <input> tag that's linked with. 

<label for="user-id"></label>
<input class="login-input" type="text" name="id" id="user-id" placeholder="아이디" required>
<label for="user-pw"></label>
<input class="login-input" type="password" name="pw" id="user-pw" placeholder="비밀번호" required>
Enter fullscreen mode Exit fullscreen mode

I thought it would be okay because I added placeholder but there are screen readers doesn't read placeholder. Therefore, label text must be written like below.

<label for="user-id" class="sr-only">아이디</label>
<input class="login-input" type="text" name="id" id="user-id" placeholder="아이디" required>
Enter fullscreen mode Exit fullscreen mode

However, the text was written as placeholder inside input and I didn't want the label text to be appear on the top of input field. So, I gave label elements class="sr-only"

.sr-only {
  position: absolute;
  left: -9999px;
  width: 1px;
  height: 1px;
  overflow: hidden;
}
Enter fullscreen mode Exit fullscreen mode

This will take the text out of screen but it's still in the html so the screen reader will still read it. (I've wrote about this a few days ago: Web Accessibility

Top comments (0)

Read next

thepracticaldev profile image

Caption This! 🤔💭

dev.to staff -

vincanger profile image

🥇The first framework that lets you visualize your React/NodeJS app 🤯

vincanger -

kuzjka profile image

Unsubscribing from every infinite observable... a good habit or overkill?

Ihor Kuzmanenko -

ssukhpinder profile image

Pantry Chef - Revolutionizing Home Cooking with AI

Sukhpinder Singh -