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>
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;
}
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;
2) Give clear information when writing img's alt value.
<img class="logo-img" src="src/img/weniv.png" alt="logo">
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>
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>
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;
}
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;
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>
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;
}
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>
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>
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;
}
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)