Looking to hire developers?
@rafaelmaiach
@fredrikjohanssonnn
Submitted
Would like feedback on the app.js file. How I could have solved the price update easier without Template Literals. Also how I can control the switch button with spacebar. Since if you know change the state with the spacebar. The priced isn't updated. Which means that if you change the switch to annually with the spacebar, the monthly prices is till shown.
@rafaelmaiach
Posted
Hello Fredrik, congrats on your solution.
About the feedback you asked for:
click event. Try to add a second event listener to listen for the space key.About the Template Literals you did. I think it's a good workaround to have the elements changed, but, you could simplify it just changing the values you need to change. Your templates are too long, because you replace the whole information. Try to replace just the piece of element you need.
@swimshahriar
Submitted
@rafaelmaiach
Posted
Hey Shariar, congrats on your solution.
As a feedback, check the font for the title, as it is still different from the challenge.
Talking about the question you made, you can take a look at CSS Grid. With CSS Grid, you can define some areas and place your boxes inside those areas, or you can use the grid layout (rows and columns) to place the boxes starting from a specific row/column. This way you will be able to position the boxes and create that challenge effect.
For example:
.card-div {
display: grid;
grid-template-rows: repeat(4, 1fr);
grid-template-columns: repeat(3, 1fr);
}
.supervisor {
grid-row: 2/4;
grid-column: 1;
}
.team-builder {
grid-row: 1/3;
grid-column: 2;
}
.karma {
grid-row: 3/5;
grid-column: 2;
}
.calculator {
grid-row: 2/4;
grid-column: 3;
}
@patricktouchette
Submitted
this was a tough one.
What would you do for this design on wide resolution (1920px and +) ? The illustration seems to be way too far on the right with a big gap in the middle.
@rafaelmaiach
Posted
Hey Patrick, congrats on your solution.
About the design question, you could set a max-width for your container. As you already is setting margin: 0 auto, when you define the max-width property, you will have your content centered on screen to a max size you define
@yaakoubox
Submitted
still phone version dose not work with me
@rafaelmaiach
Posted
Hey great solution. The mobile version worked here for me. Maybe you fixed it.
@sauravchamoli17
Submitted
Any suggestions and comments are welcome!
@rafaelmaiach
Posted
Hello Saurav, congrats on your solution!
As some feedback:
Your layout isn't responsible for small and higher default resolutions. Maybe you can study about media queries to improve the layout for smaller resolutions and set a max-width and margin: 0 auto for higher resolutions, so your content will always have a max size layout and fit better on screen.
@dianastanciu
Submitted
This was my first project ever using React. I struggled a bit, and I know it is not perfect, but I am happy I managed to do it, since I learnt quite a bit. If anyone has any recommendation as far as best practices in React go, please do tell.
@rafaelmaiach
Posted
Hey Diana, great solution. Using React was a great choice.
About some feedback:
First, I would check on the report problems about accessibility and try to fix them.
It has some time since I used React for the last time (I'm more into Vue now), but I can give you some feedback.
About styled components, I would declare them below the main component render. I prefer to open the code and see my component first instead of a list of styles.
When I checked the component Countries-list.js, it looks you could have splitted the Loading component in a separeted file. Also, even inside the renderData function, for example, the CountryCard, maybe you could have a separated component that receives the dataList and this component takes care about the logic to render the country card.
On country details, I saw some CountryDetails repeated, maybe it's a good option for a separated component too.
Well, as a more advanced feedback, for your future studies, you could check about Redux to centralize your app state and your services functions (api calls)
Keep the hard work and congratulations again
@dagonmar183
Submitted
This is my first project in Frontend Mentor. I made this landing page using anly html&Css, I used Flexbox to make responsive. Feedback is welcome!
@rafaelmaiach
Posted
Hey Dani, congrats on your solution. Very well implemented.
As some feedback:
You could try to use different tags on your html, not only divs for containers. Maybe some sections, for example, would have a better semantinc.
As I have a ultrawide monitor, your solutions get a lot of elements with large spaces or their width are very big. I don't have a thought about the best solution for this case, because I'm still studying about, but maybe you could have a max-width for the elements, with the margin: 0 auto for the page. You can try different approachs and resolution and see what fits better
Check the accessibility problem on the report and fix it, it's a good way to practice accessibility
Maybe you didnt style the border for the Get Started button, you could check the challenge image again and improve the border layout, would looks better.
@fadzrinmadu
Submitted
please review my code and let me know what i need to improve. thanks in advance
@rafaelmaiach
Posted
Hello, congrats on your solution. Very well done! As I checked, you used different html tags to correspond with what you were building and used them correctly, this is great
For a feedback, I would check the wave effect you made, at least for me, when I check the preview, it isn't being shown correctly. Also, the images seems to lose resolution when rendered too big for me as well.
As I use a widescreen monitor, the text elements have their width bigger. Maybe you could define a max-width value, so you can have smaller blocks and would be better to read the information.
@waynefox
Submitted
All comments are welcome
@rafaelmaiach
Posted
Hey Wayne, congratulations on your solution! Clean and well done.
About some feedback:
Comparing the solution with the challenge, it seems some font-sizes and elements spacing (padding / margin) are different.
Also, your buttons don't have a cursor: pointer set. This makes bad feedback to user when hovering them.
@screaminghaiku
Submitted
I'm just starting out and would like some feedback on how I can better my syntax, clean it up, and any other tips.
I'm still figuring out where to use vw vs % vs rem vs more fixed units like px.
Thanks for checking out my effort.
@rafaelmaiach
Posted
Hey, congrats on your solution!
As some feedback I can give:
Prefer IDs/classes to style your elements instead of inline styling. Inline styling can be very hard to read when you need to use a lot of styling.
You could check some different resolutions when resizing the browser to check some layout broking gaps.
@umutbozdag
Submitted
What should i do to optimize my code? Any suggestions?
@rafaelmaiach
Posted
Hey, congrats for your solution.
Some feedback I can give:
I would replace alert("Email Address can't be empty!"); to an error message below the input field. This prevents the page from being blocked by alert and the user has a cleaner and more good looking feedback
What I like to do when I have some conditions inside a function is to put the invalid conditions first and just return if they are matched, so the good scenary can be written in an easier way to read. For example:
function validateEmail(e) {
if (input.value == "") {
// do something
return;
}
if (input.validity.typeMismatch) {
input.style.border = "2px solid hsl(0, 93%, 68%)";
invalidText.style.display = "block";
iconError.style.display = "block";
} else {
input.style.border = "1px solid hsl(0, 36%, 70%)";
invalidText.style.display = "none";
iconError.style.display = "none";
input.value = "";
}
}
@ninoschelcher
Submitted
Not a javascript master, let me know what i could improve!
@rafaelmaiach
Posted
First, the reproduction of the challenge is very good. Congratulations. About the Javascript part, I'm not a master too, but I can give some feedback about what I saw.
On the file script.js, you first declare some variables that hold the elements you want to work with on the eventListener to check the email input and then you use these elements on the listener callback.
As a best practice, if you don't need to change the value of a variable, prefer to use "const" instead of "let". So, if you are getting the elements, it seems you will not want to replace that variable with other content, so use const for them.
On checkInput function, as a good practice I do, is to receive a first parameter as the event parameter. On your case, you are accessing a global event variable, but I think JS or the browser is smart to know that event is the one for the button click
Prevent from create a chain of assignments, like: error = document.getElementById("error").innerHTML = "This email is not correct"; The problem is that become hard to read and you are changing the variable declared to be your element. You could have done something like error.innerHTML = "This email is not correct" because error is already declared as your element.
