@Ahmed96Mah
@ponhuang
Submitted
Found it quite challenge to make the share icon and share container to align center. Also having problem to change the SVG color, had tried fill, stroke and color, none of them are working. So in the end, I set brightness and grayscale. Is there any better solution to create these share section to expert code?
@Ahmed96Mah
Posted
Hi Pon,
How are you? Hope you are doing well π
The reason that using align-items doesn't center the "share-icon-box" div, is because the "share-icon-box" div (which contain the arrow image) isn't actually contained within the "share-container" div. So, using the align-items property won't have any effect on an element that doesn't existing inside of the "share-container" div.
So, you can solve this problem manually by increasing the margin-top property of "author-container" div to 4.5rem
@media screen and (max-width: 62.5em)
.author-container {
margin-top: 4.5rem;
}
As for the coloring issue, the fill property won't work on the SVG because, the SVG is used within an img tag. So, you will have to use the filter property.
Here is a tool on CodePen by Barrett Sonntag, through which you can set the color of any image using the filter property.
All what you have to do, is to enter the hex value of the color you want (in the Target color input field) and click on "Compute Filters" button and it will generate a filter value for you with the color you want.
Note: Barrett states that,
" For this code to work well the starting color needs to be black. If your icon set isn't black you can prepend "brightness(0) saturate(100%)" to your filter property which will first turn the icon set to black. "
Which means that if the SVG's initial color isn't set to black, you will have to add "brightness(0) saturate(100%)" at the start of the generated value for filter property.
And by the way, don't forget to generate another screenshot of your updated solution.
Hope this helps.
Have a great day π
Marked as helpful
@ponhuang
Submitted
This is quite challenge me to understand the email validation in JavaScript and the hero-img. Until now I still couldn't figure out how to solve the issue of background image and hero-img size in desktop and mobile device. :(
I will appreciate that if you can give me any advice or idea to make this solution better.
@Ahmed96Mah
Posted
@ponhuang
Hello, Pon :)
Ok, to make the layout more similar to the design, I used flex-box which is much...much easier.
Note: Always remember to add node_modules folder to the gitignore file because of its size.
So, instead of writing a long review with the modifications, I will link the modified code and mention the most important points in it:
1- This is a modified version of your code on google drive. You should ignore (__MACOSX) folder.
Note: I have ONLY commented out the lines that I didn't need and wrote my modifications directly below each block so that it would be more easy for you to notice the changes.
2- The most important notes are:
#mobileImage {
display: none;
}
However, this gets overridden by the mobile styles (in _media.scss) with:
#mobileImage {
display: block;
width: 100%;
}
&__img {
display: none;
}
For the e-mail validation, I replaced the regex (regular expression) with what I believe to be a better one, which I found from one of Adriana's solutions.
It is shorter, in case you find it difficult to understand, this is its meaning:
const regexEmail = /^\w+([\.-]?\w+)*@\w+([\.-]?\w+)*(\.\w{2,3})+$/;
// ^ this symbol represents beginning of input
// \w this means any upperCase character, lowerCase character, number or underscore
// + means: one or more characters
// ([\.-]?\w) means, a dot '.' or hyphen '- '. ('?' means: but there must be a character after it)
// * means: zero or more combinations of ([\.-]?\w)
// @\w+ means: @ and one or more characters after it
// (\.\w{2,3})+$ this means at the end, there must be a dot, then, 2-3 characters like .com, .eg, uk or .net
// $ this represents end of the input
Today, i used the following links to understand how to use regular expressions with match:
Note: there are other smaller style properties which was added to the hero image, hero_info div and so on just to resize the elements.
Hope this helps :)
If you have any questions, don't hesitate to ask. Really :)
Have a nice day.
Marked as helpful
@ponhuang
Submitted
Deeply appreciate any feedback or suggestion to improve responsive design or else in my solution. :)
I used Google Chrome to inspect the mobile version, it seems ok. However, in the real mobile device, the social link doesn't have space below, how will you manage it? Adding padding-bottom?
@Ahmed96Mah
Posted
Hello Pon,
How are you?. Hope your are doing well :)
I checked your design on my mobile and it looks fine. I think there will always be a space between the social icons' div and the attribution paragraph, as you have set the gap between the two elements to 3rem (in your file _layout.scss on line 146). If you mean that, you want to have space below the div with class 'attribution', then you could use margin-bottom to set the space that you want.
However there are points that I have noticed:
1- If you try to rotate the page on mobile, you will find that the footer is placed on top of the main. The reason for that, is the height you have provided for main's section with class 'section-hero', as you have set that section to have a height of 60vh (in your file _layout.scss on line 16). However, a height of 60vh for a rotated mobile screen isn't enough for the section to display its content correctly (which makes the section's content spill out of its container).
So, you can change that property as follows, and it will have the same effect (but solve the footer's position issue):
/* change the property of height (on line 16) to min-height */
min-height: 60vh;
// This will ensure that your section has a minimum height of 60vh
// But also allows that section to expand for any narrow device (like a mobile)
2- Always avoid setting an element's width to 100vw (in your file _layout.scss on line 147), and then giving it a right & left padding or margin (as you did in the same file on line 69). Because this will always results in a horizontal overflow (unless you contained the element's width). And It is also, better to use 100% (instead of 100vw) when you want to set an element's width to full-screen width. Because even the browser's vertical scroll bar (which appears by default on some browsers) is considered as part of the screen's width, and will result in an unwanted horizontal scroll bar to appear (if you choose to use 100vw to set an element's width instead of 100%). So, you could do the following:
/* This is the easiest solution that you could follow */
// change the footer's width property to max-width instead of using width on line 147
max-width: 100vw;
// this will maintain the padding you have set for the footer, but will decrease the footer's width, so that the footer with (all the padding and margin) would still have a max of 100vw.
// However, keep is mind that if a mobile or a tablet has the vertical scroll bar enabled, this will make a horizontal scroll bar appear at the bottom of the screen (so it is better to use max-width: 100%).
3- To make the hero image centered correctly, you could set the image width to 100% instead of 90% (line 23). And if you want the image to be bigger, you could increase the container's width limit from 90% to maybe a 95% (on line 21) as follows:
.hero {
&__img {
width: Min(95%, 60rem); // limit changed to 95%
img {
width: 100%; // changed to 100%
}
}
....
}
Also, notice that there is a very tiny overflow because of the (right & left) padding that you have set for 'hero__info' div (it is barely visible though) like a centimeter :)
Hope this helps.
Take care, and have a nice day :)
By the way, don't forget to generate a new screenshot of your solution after you update it through the update solution section. So that you are able to see your updated design.
Marked as helpful
@Batareika007
Submitted
JS script work, but I didn't figure it out how to make it work both, submit form and add class without reload the page..
@Ahmed96Mah
Posted
Hello Alexei,
If you mean that you want the user to be able to submit another review without needing to reload the page, then you should set a timeout function to your thankU function (located at line 14 of script.js) as follows:
// notice that you should write const or let before the function name
const thankU = () => {
let selected = document.querySelector('input[type=radio]:checked');
thankUCard.classList.add('active');
starResult.innerText = selected.value;
setTimeout( () => { // add a setTimeout function to your code
// Then, remove the 'active' class from the card
thankUCard.classList.toggle('active');
}, 2500); // This sets a delay of 2500 millisecond or 2.5 seconds
return false;
}
So, after the card appears, it will stay there for 2.5 seconds and then disappear which will allow the user to submit another rating.
Hope this helps. If you have another question, don't hesitate to ask.
Have a nice day :)
@TjasaZil
Submitted
Made with Tailwindcss and Vue, tested with Jest. Any feedback is welcome :)
@Ahmed96Mah
Posted
Hello TjaΕ‘a,
The following ideas could help you for the mobile styles:
1- There is no need to use the position property, as the following combination affects how the image is displayed and also the layout of the '<main>' element (which will crop the top and bottom of the main container)
// So, the following lines could be removed
body {
position: absolute;
left: 50%;
top: 50%;
transform: translate(-50%, -50%); // This causes main to be cropped
overflow: hidden; // This doesn't allow you to scroll to see the remaining of the main
}
Note: if your intention of using the position property was to display the container along the whole screen (without the need to scroll), then you will have to minimize the heading's font sizes, and elements paddings and margins to achieve that goal.
2- You could leave a bit more space around the main element by setting the main's width with a vw (viewport width) unit:
main {
width: 90vw; // Instead of using 100%
}
3- For the image, my personal opinion is that, it would have been a lot easier to deal with an image tag rather than using a div's background as an image. However, with your current solution, you can change the image's height to a more favorable value:
.image {
height: 17rem; // the height has changed from previously set value of 23rem.
}
4- To make the button aligned correctly with the text, you could change the margin-left property (located at line 135 in main.css) to 30px.
Hope this helps.
If you have any questions, don't hesitate to ask.
Have a nice day :)
Marked as helpful
@ponhuang
Submitted
Use the design file in Figma to get the accurate size. I deeply appreciate any feedback or advice :)
@Ahmed96Mah
Posted
Hello Pon,
Your desktop design looks awesome, However, there are certain style properties for desktop that conflicts with the mobile appearance of your site. The following rules should help you with that :)
1- You should overwrite the margin property for '.main-section' class as follows
// this is located inside _midea.scss partial file
.main-section {
gap: 4.9rem;
margin: 11.8rem 0rem; // <========= This is the new line
}
notice that to center the elements inside the main tag, we would set their widths using vw (viewport width) unit to achieve the wanted effect. (see below)
2- You should set the header tag to take all the available screen width, as follows:
// this is located inside _midea.scss partial file
.header-section {
text-align: center;
flex-direction: column;
justify-content: center;
align-items: center;
gap: 3.9rem;
width: 100vw; // <========= This is the new line
}
3- You should add the following to your _midea.scss file, to set the width of the div (with a class review )
// this should be located inside _midea.scss partial file
.review {
width: 80vw;
}
4- You should change the padding for class 'rate', so that it doesn't overflow out of its container element.
// this is located inside _midea.scss partial file
.rate {
padding: 1.5rem 0rem 1.5rem 0rem; // <========= This is the updated padding style
}
5- You should set the width for the author div as follows:
// this is located inside _midea.scss partial file
.author {
width: 90vw; // <========= This is the updated width style
padding: 4rem 0rem 0rem 0rem; // <========= The padding should be updated as well
}
6- You can add some margin for author__container div element:
// this is located inside _midea.scss partial file
.author {
&__container {
margin-left: 3rem; // <========= The line should be added
}
}
7- You should also add styles in _midea.scss for testimonial__text paragraph as follow:
// this is located inside _midea.scss partial file
.testimonial {
&__text { // <========= The line should be added
padding: 0rem 3rem; // <========= The line should be added
} // <========= The line should be added
}
This should fix mobile and tablet designs. Hope this helps :)
If you have any questions, don't hesitate to ask.
Have a nice day :)
Marked as helpful
@Adriana2710
Submitted
Hi team,
I'm still very insecure about JavaScript, but let's keep practicing to improve it. Feeling more comfortable with HTML and CSS. :)
Feedbacks are welcome!
@Ahmed96Mah
Posted
Hello Adriana,
You have done an awesome job :)
If you would like to slightly improve your code, you could do the following:
1- You could use arrow functions instead of regular ones (since you already used it in the event listener function). So, it would look as follows:
const checkInputs = () => {
const firstNameValue = firstName.value.trim()
.... and, so on
}
const setErrorFor = (input) => {
const trialControl = input.parentElement;
..... and, so on
}
const successValidation = (input) => {
const trialControl = input.parentElement;
..... and, so on
}
2- There is a very small change you can do for the page's styles. The error text (colored in red) is align to the right. If you would like to fix it, you could change the following line in your styles.css file:
.trial__control p.trial__error-txt {
// line 90 in styles.css
right: 16px; // This should be changed to ==> left: 16px;
}
3- Another thing that you could add, is a click event listener to the email input field, so that if the user try to re-enter their email (if there was an error in the previous attempt), the email's error message would disappear to allow the user to write their e-mail freely. So, it would look as follows:
// You already have this line in your code
const email = document.getElementById('email');
// You need to add this one, though
const errorParagph = document.querySelector('.trial__error-email');
const processClick = () => {
if( email.parentElement.classList.contains('error') ) {
email.parentElement.classList.toggle('error');
// The above line will remove the error class from e-mail's parent div (which is
responsible for showing the email's error paragraph)
}
};
email.addEventListener('click', processClick);
/*
next line is required to ensure that, if the user also clicks on the space occupied by the error paragraph it will also count as a click on the input field
*/
errorParagph.addEventListener('click', processClick);
4- (This is a general note, but it doesn't necessarily apply here) Remember that, if you want to add the same event listener to multiple elements in the page, it is best practice to use a callback function instead of a listener function expression. For example:
if you have multiple forms and you want to add a "submit" event listener to each one it would be best to do it as follows:
const forms = document.querySelectorAll('form');
const callbackFunc = (e) => {
isValid = true;
checkInputs();
if (!isValid) {
e.preventDefault();
//preventDefault vai cancelar o evento padrΓ£o do browser
}
}
forms.forEach( (form) => {
form.addEventListener("submit", callbackFunc);
// So the two forms will have the same callback function.
} );
// or you could do it with a for of loop
for (const form of forms) {
form.addEventListener("submit", callbackFunc);
}
Hope this helps.
If you have any questions, don't hesitate to ask.
Have a nice day :)
Marked as helpful
@Batareika007
Submitted
JS script not great, but it works ) I will be glad for a hint how to write it better if possible. Thanks
@Ahmed96Mah
Posted
Hello Alexei,
These steps could generally improve your JavaScript code:
1- You could use arrow functions instead of regular ones. For Example you could say:
const toDark = () => {
toggle.classList.remove('white-theme-toggle')
....
}
2 - You could use const instead of let for the first 5 lines, as it is always best practice to use it when possible
3- You could use a callback function instead of a function expression for the event listener. for example:
const toggleMode = () => {
if (toggle_ball.classList.contains('toggle__ball_dark')) {
....
}else {
....
}
};
toggle.addEventListener("click", toggleMode);
4- You could use foreach loop or 'for of' loop instead of regular for loops .
card.foreach((item) => {
item.classList.toggle('white-theme-card-bg');
item.classList.toggle('dark-theme-card-bg');
});
And generally, I think that you could merge both "toDark" & "toWhite" functions into one function and use 'toggle' instead of "add & remove" since the website will always have an initial style and all what you want is to flip or "toggle" the classes.
Have a nice day ;)
Marked as helpful
@schwalbj
Submitted
I have used bootstrap to achieve responsive design. Can you point me into a direction what I have to look at in order to achieve the same without bootstrap?
My main challenge was to somehow center the main content horizontally. I have achieved this by applying different top margin, depending on the breakpoints. Is there a best practice how to achieve this?
@Ahmed96Mah
Posted
Hello Janina,
To center a container inside the middle of the screen you can use CSS's flex-box property. For example,
If you have a container div inside of the body tag:
body {
display: flex; // using flex-box
flex-flow: column nowrap; // defining main flex-direction (column) and not allowing item wrap
align-items: center; // For aligning items along the cross axis
justify-content: center; // For aligning items along main axis
min-height: 100vh; //ensuring that the body's height is at least 100 view height
}
If you have multiple containers they will be all centered, and aligned vertically (as flex-direction is set to column). Similarly, To align items horizontally while being centered as well, you could use:
body {
display: flex; // using flex-box
flex-flow: row nowrap; // defining main flex-direction (row) and not allowing item wrap
align-items: center; // For aligning items along the cross axis
justify-content: center; // For aligning items along main axis
min-height: 100vh; //ensuring that the body's height is at least 100 view height
}
So, if you have multiple containers they will be all centered, and aligned horizontally.
Note: setting the body's height property, doesn't contribute anything in centering the item, however, it allows enough space (if you are dealing with small containers) for you to see where the item is placed. So, you don't necessarily need it.
you can look up the property on MDN (which is an amazing reference) @ : MDN
It is best to always use flex-box for aligning items all over the page. While to provide the actual page's layout (such as deciding how tall the header, main & footer would be), you should use CSS's grid property. Read about it here @ MDN
Have a nice day, and keep up the good work :)
Marked as helpful
@natasha-crain
Submitted
I recreated this webpage as part of my course with The Learning People. There are a few things still not quite right:
@Ahmed96Mah
Posted
Hello, Natasha!.
I am Ahmed from Egypt.
First of all, you have done an awesome job. I have given your code a quick look, and I would like to help you understand the points in your code that are causing the problems you have mentioned :)
1- The cart problem: You might think that your code isn't adding new items, since it looks like that it is ONLY adding the current selected element. However, that is not true!.
Think about the following case:
You decided to add one item to cart by pressing + one time, and hit the add to cart button. So, your global count variable will increase by 1. While the global cost variable will increase by $125.
Then, you increase the count to 2 (by hitting + again), and hit the update cart button. However, this time instead of adding 2 items (which should make the total equal 3), it will only add one (and the total will be equal to 2). You see, when you hit the + symbol, the count only increase by one as you have programed it to do that ( in addUnit(), minusUnit() ). So, it doesn't actually take into account the number chosen (the number that appears between (+ and -)).
So, you could add another 2 global query Selectors to your code, which selects the span (with id= "cartNum"), which should contain the actual value you want to add, and the span (with id= "itemTotal"). So, your addToCart() function could look like the following:
// First, add the 2 global selectors
// this is the container of current number selected
const numOf_Items = document.querySelector('#cartNum'); // or use getElementById :)
const previous_Selection = document.querySelector('#itemTotal');
// this is the amount already in the cart
// And This could be your new addToCart()
const addToCart = () => {
const actualCount = parseInt(numOf_Items.innerText) + parseInt(previous_Selection.innerText);
cartCount.innerHTML = actualCount;
}
and your itemUpdate() function could look like the following,
const itemUpdate = () => {
const actualCount = parseInt(numOf_Items.innerText) + parseInt(previous_Selection.innerText);
itemTotal.innerHTML = actualCount;
const actualCost = actualCount*125; // This should be the correct price :)
priceTotal.innerHTML = `$ ${actualCost}.00`;
}
This should hopefully fix your cart problem ;)
Note: It is important to note that this doesn't affect the existence of your 2 functions (addUnit, minusUnit) as they still serve the function of keeping the items below 10 and also keeping them above 0. So, don't remove them, they just won't be used to determine the number of items selected by the user or the total cost.
2- Arrow's Css Problem:
This is a lot more straight forward. Don't worry :)
in your css file (lines 345 to 348).
What you have written means the following: For each image in any element, which has a class of "arrows", change the color of the SVG when a mouse hovers over it.
this is cool for the arrow itself, but to make the effect happens when a mouse hovers over a the circle as well, you will need JS.
A possible solution would be:
document.querySelectorAll('.arrows-pop').forEach(n => n.addEventListener('mouseenter', (e) => {
e.target.querySelector('.arrow-hover').classList.toggle('highlight');
// So, you will have to make a new class named 'highlight' which has the same two CSS lines (lines 346 to 347).
// This will add the coloring effect to the arrows when the mouse enters the div
});
// and also
document.querySelectorAll('.arrows-pop').forEach(n => n.addEventListener('mouseleave', (e) => {
e.target.querySelector('.arrow-hover').classList.toggle('highlight'));
// This will remove the coloring effect to the arrows when the mouse leaves the div
});
Phew, hope this helps you. And keep up the awesome work.
If there is anything not clear, due to language barrier (as English isn't my first language, I am more than happy to elaborate more).
Wish you all the best :)