-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jesse 1ste pull request. get routes werkend #25
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super bezig Jesse! Je hebt echt al veel gedaan, topper 🚀
Ik heb een paar comments gezet bij je code, maar heb je code nog niet getest verder door het lokaal te draaien. Zal dat ook nog even doen! :)
Dankjewel voor de feedback. Ik ben nu van start gegaan met het verbeteren van mijn werk |
@nikoladupreez alle feedback is nu verbeterd. ✅ in de volgende push zie je uiteindelijk alle verbeteringen |
ver 1.2 is gepushed hier zijn nog wat andere dingen waar ik mee bezig ben
volgende stap Mijn volgende stap is om deze pagina's helemaal af te krijgen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heel nice dat je zo snel de feedback hebt verwerkt Jesse! Ziet er meteen cleaner uit ✨ 🧹
Ik heb ook even een test gedaan door het lokaal te draaien en krijg op het moment een 500 error wanneer ik een rating probeer toe te voegen voor een listing. Heb jij dat ook?
public/script.js
Outdated
fetch(this.action, { | ||
method: this.method, | ||
body: new URLSearchParams(starItem), | ||
}).then(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je hebt op het moment niets in de .then
staan, hoort de code voor de succes state wat hierna komt daar eigenlijk in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je zou dan ook nog een .catch
kunnen toevoegen om een error state te laten zien (is een nice to have, niet belangrijk voor je user story)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let formSubmit = this.querySelector("button[type='submit']");
formSubmit.classList.add("succes");
formSubmit.innerText = "Gelukt"; // Change the text of the button to "Gelukt"
formSubmit.disabled = true; // Disable the button
dit is wat na de then komt. er komen bepaalde styling die de succes state aangeeft. Ik zal kijken of het mij nog lukt om die catch toe te voegen. We zijn nu namelijk bezig met alles voor te bereiden voor de sprint review.
(ik krijg trouwens geen error wanneer ik de post uitvoer van de star rating)
public/script.js
Outdated
}).then(); | ||
|
||
// Wanneer het item succesvol is verzonden, voert hij het volgende uit | ||
let formSubmit = this.querySelector("button[type='submit']"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Op het moment wordt de succes state altijd toegevoegd, ook als de request niet succesvol was :)
Ik krijg namelijk een 500 error wanneer ik een rating probeer toe te voegen, maar ik zie wel de succes state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heb er nu dit van gemaakt. Bij de then voert hij nu een functie uit waarbij hij kijkt of het gelukt is of niet.
als het gelukt is voert hij de succes state uit en zo niet dan voert hij de error state uit waar de tekst dus naar mislukt veranderd.
starForms.forEach((starForm) => {
starForm.addEventListener("submit", function (event) {
// Maak een FormData-object van het formulier
let starItem = new FormData(this);
// Voeg 'clientside' toe aan de FormData
starItem.append("clientside", true);
// Voer het fetch-verzoek uit naar de server
fetch(this.action, {
method: this.method,
body: new URLSearchParams(starItem),
}).then(() => {
// Wanneer het item succesvol is verzonden, voert hij het volgende uit
let formSubmit = this.querySelector("button[type='submit']");
formSubmit.classList.add("succes");
formSubmit.innerText = "Gelukt"; // Verander de tekst van de knop naar "Gelukt"
formSubmit.disabled = true; // Schakel de knop uit
})
.catch(() => {
let formSubmit = this.querySelector("button[type='submit']");
formSubmit.innerText = "Mislukt";
formSubmit.disabled = false;
});
// Voorkom standaardgedrag van het formulier
event.preventDefault();
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jesse,
Je hebt super goed en compleet werk geleverd! Afgezien van de comments van mijn collega heb ik maar twee kleine opmerkingen over accessibility.
Heel goed om te zien hoe je zo gestructureerd te werk bent gegaan, netjes met goed beschreven commits.
De pagina zelf ziet er ook goed uit en je hebt er een leuke twist aangeven met de animaties!
@@ -0,0 +1,12 @@ | |||
<div class="stars"> | |||
<input type="radio" id="rating-1-<%= type %>-<%= houseId %>" value="1" name="rating[<%= type %>]" aria-label="1-ster" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dit is een prima oplossing voor de stars! Let er op dat de aria-label
die je definieert hetgeen is wat mensen te lezen of te horen krijgen als ze gebruik maken van hulptechnologieën. Zorg er dus voor dat dit voor mensen geschreven is. Dus niet "1-ster
" maar beter "1 ster"
of "Geef 1 ster"
.
<% lists.forEach(list => { %> | ||
<li> | ||
<a href="/lijsten/<%= list.id %>"> | ||
<img src="/images/download.svg" alt="afbeelding-huis" height="200" width="200" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aangezien deze afbeelding niets meer dan een icoon is voor deze link, en daarmee een "decorative image", zou ik hem geen alt
-text geven, maar deze leeg laten: alt=""
.
Zie https://www.w3.org/WAI/tutorials/images/decorative/
Als een gebruiker met hulptechnologieën focust op deze link krijgt deze al "Bekijk Lijst" te zien of te horen.
} | ||
} | ||
|
||
.wrapper-house ul .houseDetail form button[type="submit"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deze button is ook op een gegeven moment disabled
, die zou ik ook nog stylen.
Hier is mijn eerste pull request.
wat is mij gelukt
het is mij gelukt om de juiste data op te halen voor de pagina die alle lijsten weergeeft en de huizen die in een lijst zitten.
server.js
In de server heb ik voor gezorgd dat hij alle lijsten worden opgehaald voor lijsten.ejs. die zitten hier in directus f_lists
Wanneer je op een zo'n lijst klikt opent hij lijst.ejs en zorgt hij dat je alleen de huizen ziet die in die lijst staan .
(Gedaan met app.get(lijsten/:id))
lijsten.ejs
Hierin heb ik een
ul
gemaakt waarin een for each loop zit. Met dat worden alle lijsten omringt met het *in die
<li>
zit er voor nu een test image + de titel van de lijst en een form met button die je naar zijn lijst pagina leid met zijn huizen.lijst.ejs
Ook hier werk ik met een
ul
waarin de huizen geloopt worden en omringt zijn met het<li>
element. In die li zie je het plaatje van het huis en nog wat informatie erover zoals de stad waar hij inzit en de prijs bijvoorbeeld.overige changes
screenshots
lijsten.ejs
Je ziet hier alle lijsten met daarbij de titel, een test image en een button die je brengt naar alle huizen die in zijn lijst zitten
lijst.ejs
dit is wat ik krijg als ik op de button klik van de lijst met de titel Amsterdam. Vervolgens zie je alle huizen die bij deze lijst horen.