Skip to content

Commit

Permalink
html and js fix, some css fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@Theblackhole0614
Theblackhole0614 committed Jul 13, 2023
1 parent 576133f commit 6766851
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 83 deletions.
14 changes: 7 additions & 7 deletions home.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,20 @@
<body>
<div id="background-popup-addcard">

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

There's multiple accessibility considerations for a popup like this, and the easiest is to use a <dialog> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

In particular, look at the usage considerations with a form https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#usage_notes
<section id="popup-addcard">
<button id="close-popup-addcard" aria-label="close popup">
<button id="closer-popup-addcard" aria-label="close popup">
<img src="./assets/close.svg" alt="close popup" aria-hidden="true">
</button>
<h2>New Card</h2>
<form id="popup-addcard-form">
<form id="form-popup-addcard">
<label for="title">Title</label>
<input type="text" id="title" placeholder="Title" name="title">
<input type="text" id="title-input-addcard" placeholder="Enter an title" name="title">

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

Currently if I click the button in the form but I've not filled out anything it still works and I get a card with empty information.
You should use the built-in form validation for that (or custom if you need) https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation#using_built-in_form_validation (particularly the required attribute)

And then you can try to have meaningful placeholders that give an example of what's expected in the field (e.g. https://www.w3.org/WAI/tutorials/forms/instructions/#placeholder-text)
<label for="username">Username</label>
<input type="text" id="username" placeholder="Username" name="username">
<input type="text" id="username-input-addcard" placeholder="Enter an username" name="username">
<label for="email">Email</label>
<input type="email" id="email" placeholder="Email" name="email">
<input type="email" id="email-input-addcard" placeholder="Enter an email" name="email">
<label for="password">Password</label>
<input type="password" id="password" placeholder="Password" name="password">
<button id="popup-addcard-submit">Add Card</button>
<input type="password" id="password-input-addcard" placeholder="Enter an password" name="password">
<button id="submit-popup-addcard">Add Card</button>
</form>
</section>
</div>
Expand Down
83 changes: 37 additions & 46 deletions script.js
View file
Original file line number Diff line number Diff line change
@@ -1,42 +1,48 @@
const items = document.getElementById('items')
const addItem = document.getElementById('add-item')
const closePopup = document.getElementById('close-popup')
const backgroundPopup = document.getElementById('background-popup')
const popup = document.getElementById('popup')
const addCard = document.getElementById('popup-button')
const title = document.getElementById('title')
const username = document.getElementById('username')
const email = document.getElementById('email')
const password = document.getElementById('password')
// Main elements
const cards = document.getElementById('cards')
const buttonAddCard = document.getElementById('button-addcard')

// Popup elements
const backgroundPopupAddCard = document.getElementById('background-popup-addcard')
const popupAddCard = document.getElementById('popup-addcard')
const closerPopupAddCard = document.getElementById('closer-popup-addcard')
const formPopupAddCard = document.getElementById('form-popup-addcard')

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023

To access a form, you should give the form a unique name and use document.forms['name-of-the-form'] https://developer.mozilla.org/en-US/docs/Web/API/Document/forms
const submitPopupAddCard = document.getElementById('submit-popup-addcard')

const titleInputPopupAddCard = document.getElementById('title-input-addcard')
const usernameInputPopupAddCard = document.getElementById('username-input-addcard')
const emailInputPopupAddCard = document.getElementById('email-input-addcard')
const passwordInputPopupAddCard = document.getElementById('password-input-addcard')

// Cancel submit
const addCardForm = document.getElementById('popup-addcard-form')

addCardForm.addEventListener('submit', event => {
formPopupAddCard.addEventListener('submit', event => {
event.preventDefault()
})

backgroundPopup.style.display = 'none'
popup.style.display = 'none'
// Popup visibility
backgroundPopupAddCard.classList.add("hiden");

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023

Typo hiden -> hidden

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

That class should be in the html directly on the popup element if you want the popup to be hidden by default

function showPopup() {
backgroundPopupAddCard.classList.remove("hiden");
}

function addAnItem() {
showPopup()
function hidePopup() {
console.log('coucou')
backgroundPopupAddCard.classList.add("hiden");
}

function addACard() {
// Add card fonctionality
function addCard() {
hidePopup()
const cardTitle = document.createElement('h2')
cardTitle.textContent = title.value
title.value = ''
cardTitle.textContent = titleInputPopupAddCard.value

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023

To get the values of the form fields, you should use FormData https://developer.mozilla.org/en-US/docs/Web/API/FormData/FormData
const cardUsername = document.createElement('p')
cardUsername.textContent = `Username : ${username.value}`
username.value = ''
cardUsername.textContent = `Username : ${usernameInputPopupAddCard.value}`
const cardEmail = document.createElement('p')
cardEmail.textContent = `Email : ${email.value}`
email.value = ''
cardEmail.textContent = `Email : ${emailInputPopupAddCard.value}`
const cardPassword = document.createElement('p')
cardPassword.textContent = `Password : ${password.value}`
password.value = ''
cardPassword.textContent = `Password : ${passwordInputPopupAddCard.value}`
formPopupAddCard.reset()
const cardLi = document.createElement('li')
cardLi.appendChild(cardTitle)
cardLi.appendChild(cardUsername)
Expand All @@ -45,24 +51,9 @@ function addACard() {
items.appendChild(cardLi)
}

function showPopup() {
backgroundPopup.style.display = 'flex'
popup.style.display = 'flex'
}

function hidePopup() {
backgroundPopup.style.display = 'none'
popup.style.display = 'none'
}

addItem.addEventListener('click', event => {
addAnItem()
})

closePopup.addEventListener('click', event => {
hidePopup()
})
// Main events
buttonAddCard.addEventListener('click', showPopup)

addCard.addEventListener('click', event => {
addACard()
})
// Popup events
closerPopupAddCard.addEventListener('click', hidePopup)
submitPopupAddCard.addEventListener('click', addCard)

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

The button is linked to the form, and triggers a submit, but pressing enter in the form also triggers a submit. This only handles the button, what's better to do in general is to directly listen to the form submit event, that takes care of any action that submits the form (i.e. effectively to create a new card we don't care if the button is pressed or not, what we care about is the form has been submitted and we got some data for the card)

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

EDIT: You might want to use the dialog events instead, see 6766851#r121662836
107 changes: 77 additions & 30 deletions style.css
View file
Original file line number Diff line number Diff line change
@@ -1,83 +1,130 @@
@import url('https://fonts.googleapis.com/css2?family=Poppins:wght@500&display=swap');

:root {
--color-blue: #083553;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

+1 for putting color variables in root
What would be even better is to use semantic variables names, instead of naming the variable based on its color, e.g. --color-primary rather than --color-blue
--color-green: #019F55;
--color-green-hover: #007e43;
--color-beige: #F5F5DC;
--color-transparent: #ffffff2a;
--color-transparent-hover: #ffffff3f;
--color-blur: #0000005e;
}

html, body {

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023

Usually html only needs a height: 100% property, and for the body, just the margin needs reset to 0, not the padding

See https://browserdefaultstyles.com/#body
box-sizing: border-box;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

You should have a reset for this that looks like this:

*, *::after, *::before {
  box-sizing: border-box;
}

There's other resets you can use, e.g. https://www.joshwcomeau.com/css/custom-css-reset/#the-css-reset-1 (the explanations in this article are also worth having a look at!)
width: 100%;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

There's no need for width 100%
(the complicated details https://stackoverflow.com/questions/28353625/why-does-percentage-width-work-even-if-no-explicit-width-value-given-for-contain/28354270#28354270)
height: 100%;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

This is fine for html, but it's restricting the body (not amazing for scrolling)
Instead of this line, you can use min-height: 100vh and that should do the trick.
(the more in-depth details jensimmons/cssremedy#70)
overflow-x: hidden;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023

Hiding overflow by default should be avoided in general, the rule is there should always be a way to see elements of the html document on the screen, even if it means scrolling. And if you want to get rid of the scrolling then you need to refactor the layout.
margin: 0;
padding: 0;
background-color: #083553;
background-color: var(--color-blue);
font-family: 'Poppins', sans-serif;
}

#background-popup {
.hidden {
display: none;
}

/*-------------------- popup addcard --------------------*/

#background-popup-addcard {
z-index: 2;
position: absolute;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

This would usually be fixed, else if there's scrolling, e.g. vertical scrolling, it will stay at the top left of the page
width: 100vw;
height: 100vh;
background-color: #0000005e;
background-color: var(--color-blur);
display: flex;
justify-content: center;
align-items: center;
}

#popup {
#popup-addcard {
z-index: 3;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023

This is not needed

Explanation: the parent #background-popup-addcard has a z-index property specified as an integer, which creates a local stacking context for its children, see https://developer.mozilla.org/en-US/docs/Web/CSS/z-index#integer
width: 450px;
height: 550px;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

These width and height enforce a size on the children, but the better approach is to let the children define the size of the parent, that would allow the site to be more responsive/flexible. (e.g. to device default font-size)
In that case these properties should be removed, and you can use padding to add some spacing between the children and the sides of the container if needed.
background-color: beige;
background-color: var(--color-beige);
display: flex;
flex-direction: column;
border-radius: 10px;
}

#popup img {
#closer-popup-addcard {
width: 40px;
height: 40px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

A better approach is to let the children define the size of the button (more flexible/responsive), so you should remove the width and height here
position: relative;
top: 5px;
left: 405px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

To position the button in the top right, you should use position: absolute, and place it based on the right side, right: 5px. In that case you would also need to specify position: relative on the parent to make the top and right offsets relative to the parent.

With position: relative on the button like now, the button still keeps taking space in the flow of the document. If you really want to keep that space at the top of the popup taken by the button, then instead of the solution with absolute, you can use:

align-self: end; /* puts the element to the right since the parent is `display: flex` */
left: -5px;
cursor: pointer;
background-color: transparent;
border: none;
padding: 0;
}

#closer-popup-addcard img {
width: 40px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

px units are not responsive in general, prefer em or rem by default.
In this case if someone has trouble reading and has increased their default font-size, it would also make sense to increase the size of the image in the button (i.e. in general it should increase the description of buttons, and in this case the image is a visual description of what the button does)
In that case you can use 2.5em for both width and height, that corresponds to 40px when using the default default font-size (which is 16px, 40/16 = 2.5).
A better approach consists of setting the icon sizes in general to 1em, and setting the font-size of the parent to what you need, in this case 2.5em

More info: https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Values_and_units#relative_length_units
height: 40px;
transition: 0.3s;
}

#popup form {
#closer-popup-addcard img:hover {
scale: 1.1;
}

#popup-addcard h2 {
margin: 0;
padding: 0;
text-align: center;
font-weight: 600;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

This currently has no effect because this font-weight for the font is missing, you need to import it: https://fonts.google.com/share?selection.family=Poppins:wght@500;600
}

#form-popup-addcard {
height: 100%;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

No need for this, the children will define the height
display: flex;
flex-direction: column;
align-items: center;
gap: 2px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

Avoid pixels, useem instead
padding: 15px 30px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

Avoid pixels, userem instead
}

#popup h2 {
margin: 0;
padding: 0;
margin-bottom: 20px;
#form-popup-addcard label {
font-size: 18px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

Avoid pixels, useem instead
width: 88%;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

If the label needs to be aligned to the left, because it's inside a flexbox, use align-self: start
}

#popup form input {
margin-bottom: 30px;
#form-popup-addcard input {
margin-bottom: 15px;
border: none;
width: 80%;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

This should be 100%, and adjust the padding on the form if needed (and it will be aligned with the label with 6766851#r121668095 and 6766851#r121663985)
font-size: 20px;
font-size: 18px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

Avoid pixels, use em instead
outline: none;
padding: 5px 7px;
padding: 10px 15px;

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

Avoid pixels, use em instead
border-radius: 5px;
border: 2px solid #ccc;
border-bottom-width: 3px;
transition: 0.2s;
}

#popup-button {
margin-bottom: 30px;
#form-popup-addcard input:focus {
border-color: var(--color-green);
}

#submit-popup-addcard {
margin-top: 15px;
border: none;
height: 50px;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023 •
edited
Loading

You can remove this and let the content define the height, adjust the block padding if needed
width: 50%;

This comment was marked as resolved.

Sign in to view

Copy link
@kevduc

kevduc Jul 15, 2023

This can be tricky for smaller screen widths, an easier approach is to let the content define the width, and adjust the inline padding, it gives similar results and is much easier to setup for responsiveness (just need to ensure the font-size is responsive)
background-color: #019f55;
background-color: var(--color-green);
font-size: 25px;
font-family: 'Poppins', sans-serif;
color: #ffffff;
color: white;
transition: 0.3s;
cursor: pointer;
border-radius: 5px;
}

#popup-button:hover {
background-color: #00cf6f;
#submit-popup-addcard:hover {
background-color: var(--color-green-hover);
font-size: 24px;
}

header {
Expand All @@ -99,13 +146,13 @@ header {
.logo-password {
height: 50px;
font-size: 50px;
color: #019f55;
color: var(--color-green);
}

.logo-manager {
height: 35px;
font-size: 35px;
color: #ffffff;
color: var(--color-beige);
}

main {
Expand All @@ -126,7 +173,7 @@ main {
margin: auto;
width: 400px;
height: 220px;
background-color: beige;
background-color: var(--color-beige);
border-radius: 10px;
animation: append-animate .4s ease-out;
}
Expand All @@ -138,7 +185,7 @@ main {
text-align: center;
margin-top: 25px;
margin-bottom: 30px;
color: #019f55;
color: var(--color-green);
}

#items li p {
Expand All @@ -165,7 +212,7 @@ main {
height: 220px;
border-radius: 10px;
border: none;
background-color: rgba(255, 255, 255, 0.164);
background-color: var(--color-transparent);

This comment has been minimized.

Sign in to view

Copy link
@kevduc

kevduc Jul 14, 2023 •
edited
Loading

A trick for colors is to create the color variables like --color-secondary: 255, 255, 255 and then to use it like for example color: rgb(var(--color-secondary)), and when you need transparency, you can do e.g. color: rgb(var(--color-secondary) / 50%)
display: flex;
align-items: center;
justify-content: center;
Expand All @@ -175,7 +222,7 @@ main {

#add-item span{
font-size: 60px;
color: #019f55;
color: var(--color-green);
transition: 0.5s;
}

Expand Down Expand Up @@ -256,10 +303,10 @@ main {
}

#add-item:hover {
background-color: rgba(255, 255, 255, 0.247);
background-color: var(--color-transparent-hover);
}

#add-item:hover span{
color: #00bd65;
color: var(--color-green-hover);
scale: 1.3;
}
}

0 comments on commit 6766851

Please sign in to comment.