@d-vinayak
Submitted
I am not able to position social icons to the right bottom side for desktop version. I will be really grateful if someone tells me what I am doing wrong plus any additional feedback will be really helpful for me
Looking to hire developers?
@tomthestrom
@d-vinayak
Submitted
I am not able to position social icons to the right bottom side for desktop version. I will be really grateful if someone tells me what I am doing wrong plus any additional feedback will be really helpful for me
@tomthestrom
Posted
As @marik999 points out, you could position them horizontally along the main axis using justify content: flex-end;
and using some margin
or padding
, right now you're positioning them to where they are using justify content: center;
, that's why they are in the center.
By the way, speaking about the social media icons, it's cool you decided to put them into a <ul>
element, that makes for some nice semantic HTML! But in my opinion it would be better to have each social media icon as a standalone list item (<li>
) instead of having just one <li>
with 3 child elements, that basically defeats the semantic approach introduced by using ul
and li
for these social media icons.
Take care, Tom
@Drallas
Submitted
This is my 3rd newbie challenge submit and I enjoyed the process of hacking this card component together. Building such a 'simple' card component isn't easy for a 'Newbie' and it requires some time to think over before trying to do it properly.
Feedback would be welcome How is the responsive experience; what can / should I improve (*see Note)? I try to use BEM but still have doubt if i used it correctly Anything else that I missed or should improve.
Note: Related to BEM I recommend this video: https://www.youtube.com/watch?v=iyR6RXUZFQ8 it's quite good and explains it in visual detail.
Where I struggle is how should elements be named when there are multiple levels of nesting.
<div class="card__mid">
<div class="card__text">
<div class="card__name">Victor Crest <span> 26</span></div>
<div class="card__location">London</div>
</div>
</div>
@tomthestrom
Posted
Hi Drallas,
here is how you could do it with linear-gradient
(and background layering):
class="card-top"
class="card"
: background: linear-gradient(to bottom, rgba(0, 0, 0, 0) 0 39%, white 39% 100%), url(./images/bg-pattern-card.svg)
;So basically you are:
Let me know if it's not clear.
Have a good day,
Tom
@Drallas
Submitted
This is my 3rd newbie challenge submit and I enjoyed the process of hacking this card component together. Building such a 'simple' card component isn't easy for a 'Newbie' and it requires some time to think over before trying to do it properly.
Feedback would be welcome How is the responsive experience; what can / should I improve (*see Note)? I try to use BEM but still have doubt if i used it correctly Anything else that I missed or should improve.
Note: Related to BEM I recommend this video: https://www.youtube.com/watch?v=iyR6RXUZFQ8 it's quite good and explains it in visual detail.
Where I struggle is how should elements be named when there are multiple levels of nesting.
<div class="card__mid">
<div class="card__text">
<div class="card__name">Victor Crest <span> 26</span></div>
<div class="card__location">London</div>
</div>
</div>
@tomthestrom
Posted
Hey, I think you're using BEM good, but since I already checked out the code a bit, I have something to add.
Creating the element with class="card-top"
could be avoided by using linear gradient for the card background. That element (card-top
) has no semantic value as it is now.
Alternatively in this case if you're styling and creating an element only for the purpose of implementing a design, try doing it with the before
and after
pseudo-elements.
margin-right: -50%;
on the card is pointless, you're already positioning it via position
and transform
.
Good luck and have a good day,
Tom
@nicole-tuznik
Submitted
No specific questions, but feedback is always appreciated! :)
@tomthestrom
Posted
Hey Nicole,
I was looking at your JS code. It's nice and readable, just a few things that are mostly a matter of taste.
This whole code could be wrapped in an IIFE
to avoid polluting the global namespace. It's a basic app and does no harm doing it the way you do, but it's a good practice to think if you really need to have globals before using them (build good habits from the beginning).
let monthlyPrice
and let annualPrice
should be const
, they are not reassigned anywhere.
const toggle = document.querySelector("#toggle");
could be a getElementById
, it tells better what you're doing since querySelector
is more of a general tool.
querySelectorAll
based on class names are not the best, it's a better practice to use data
attributes, again at this scale of an app it doesn't matter that much, just something to be aware of.
toggle.addEventListener("click", function () {
if (toggle.checked) {
for (i = 0; i < annualPrice.length; i++) {
annualPrice[i].style.display = "none";
}
for (i = 0; i < monthlyPrice.length; i++) {
monthlyPrice[i].style.display = "block";
}
} else {
for (i = 0; i < annualPrice.length; i++) {
annualPrice[i].style.display = "block";
}
for (i = 0; i < monthlyPrice.length; i++) {
monthlyPrice[i].style.display = "none";
}
}
});
This could be shortened for readability and to reduce repetitiveness to:
toggle.addEventListener("click", function () {
for (i = 0; i < annualPrice.length; i++) {
annualPrice[i].style.display = this.checked ? "none" : "block";
}
for (i = 0; i < monthlyPrice.length; i++) {
monthlyPrice[i].style.display = this.checked ? "block" : "none";
}
});
You could also use Array.map()
or Array.forEach()
instead of those for
loops to make it more declarative.
Your github repo shouldn't include the .DS_Store
file and the package-lock.json
.
Have a good day,
Tom
@EAguayodev
Submitted
Would like feedback on whether the javascript written was good code or couldve been written better.
@tomthestrom
Posted
Hey Eric, it's easy to read, I see what it means to do, good job on that. Since you asked for feedback, I have a few things to say.
Did you test it? You're using a validEmail()
function that is not defined.
Everything is in global namespace, which for this app with a few lines is alright, but you could solve it with an IIFE
.
isvalidateEmail
is not camelCase and sounds like it does 2 things, while it only does one. I would call it isValidEmail
since it returns true
or false
based on a regex.
You could extract the part where you do:
email.classList.add('error');
email.innerText = 'Email cannot be blank';
email.classList.add('error');
email.innerText = 'Email is invalid!';
into a function. You could have a place to store these error messages, and then just pass the error message to email.innerText
based on the given parameter.
const small = form.querySelector('small');
this way of selecting an element is error prone, you should either use an id
or a data
attribute. You need to be able to always uniquely identify the element you wanna manipulate. Imagine you'd have another <small>
element in the form, this code could break.
Also for the github repo, you should not be having a directory and nothing else at the root level. You should move the contents of that directory into root. While at it, I would include a .gitignore
and a README.md
file.
Have a good day,
Tom