William Nicholson• 75
@Passenger89
Submitted
Still struggling with the background images resizing on zoom in.
Manuel Gil
@ManuGil22
All comments
- Manuel Gil• 340
@ManuGil22
Posted
Hey there! Pretty cool solution! Have one thing to say as feedback:
- Always use heading tags in order! You should use <h1> first and if the font its too big u can always style it with css!
<h1 class="card__name">Victor Crest</h1>Everything else seems pretty good for me! Well done :D
0 - Hrishikesh kadam• 30
@hrishikdm
Submitted
please look out for my code and feel free to give feedback and give your suggestions thanks you
Manuel Gil• 340@ManuGil22
Posted
Hey @hrishikdm pretty cool solution! Have some things to say as feedback:
- Wrap all body content in a <main> tag for accessibility purposes.
- Always use heading tags in order. Dont use <h2> or <h5> as a first tag, you can use a <h1> instead and change the text-style with css!
<h1>How did we do?</h1>Also for the text area were u have h5 and h2!
Marked as helpful
1 - Danny• 10
@Danny-Rodriguez
Submitted
How effective is my solution to centering each element?
Manuel Gil• 340@ManuGil22
Posted
Hey @Danny-Rodriguez pretty cool solution! Have only one thing to say as feedback:
- Always use heading tags in order. Dont use <h4> as a first tag, you can use a <h1> instead and change the text-style with css!
Marked as helpful
2 - Abhishek Bhardwaj• 20
@AbhishekBh72
Submitted
I want help on the positioning of elements/div in webpage. How can I place a section or an element in the exact center of a webpage?
Manuel Gil• 340@ManuGil22
Posted
Hey @AbhishekBh72
To position a element in the exact center of a page you can use this attributes on the body element:
- height: 100vh; //To make the height of the body 100 viewheight units
- display: flex; //To use flexbox
- justify-content: center; //To center elements horizontally
- align-items: center; //To make all items center in the page vertically
I hope I answered your question! Keep pushing and happy coding!
Marked as helpful
1 - Viktor Kovács• 310
@victorsonet
Submitted
I am not sure if the media querys are correct or should I do it differently.
Manuel Gil• 340@ManuGil22
Posted
Hey @victorsonet !
Some feedback:
- Your media queries are not working properly. Maybe its better if you wrap the qr-section and content-section in one container and work with that container size. I would probably do a max-width: 350px for that container and u wont need to add queries. However u will have to change some of ur css in order to do that.
This is a link to my solution: Frontend Mentor solution
If you want u can take a look at it and ask me anything u need.
Keep pushing and happy coding!
Marked as helpful
1 - Aayush Sood• 1,175
@soodaayush
Submitted
Any feedback would be appreciated.
Manuel Gil• 340@ManuGil22
Posted
Hey @DevBaddy ! Good work there! Your repo link its not working tho, cant check the code to give any feedback. However it seems all good. Try to add a <main> tag around the code for accessibilities purposes!
Well done and happy coding!
0 - Ginaldo Capistrano• 70
@gcapistrano
Submitted
I would really appreciate feedback from the community.
Manuel Gil• 340@ManuGil22
Posted
Hey @gcapistrano your solution is pretty good! Just some details u might wanna change:
- Add a <main> tag around the main code
- Try to match colors of the desing. I think the h1 should be like a dark blue or smth like that.
Really well done!
Keep pushing and happy coding!
Marked as helpful
1 - Sofia• 40
@Escobar23
Submitted
I am new to this, I would appreciate the feedback!
Manuel Gil• 340@ManuGil22
Posted
Hey @Escobar23 good job with the challenge!
Just some details to check:
- Try to decrease the margin-top for the .card, 200px seems like too much and if u check it on the phone u will see its a lot of margin. Maybe u can try with margin-top: 8rem
- Add a <main> tag around the div with card class to solve the accessibility issue.
Everything is just perfect, dont worry about the sizes of the card, there are just details and u will improve it for sure!
Keep pushing and happy coding!
Marked as helpful
0 - John Raily Doe• 50
@hopzinga22
Submitted
This project was quite difficult but I managed to complete it.
Any criqs will be appreciated.
Manuel Gil• 340@ManuGil22
Posted
Hey there @hopzinga22
Have some feedback for you.
- Dont use <h4> tags for the price and the time left. Heading levels should increase by one. U should use <h2> or maybe smth else like a <p> or <span>. Also try to fix this on the creator section.
- When hover on the nft-name and the creators-name change the cursor to pointer and the color of the text. Can do it like this for the nft-name:
h1:hover { cursor: pointer; color: (#cyan color); }And for the creators-name u can add a span around it:
- html
<h5><span>Creation of </span><span class="creator-name">Jules Wyvern</span></h5>- css
.creator-name { cursor: pointer; color: (#cyan color); }- You can also add this lines to class time-amount in the css file to make the time left move to the right:
.time-amount { display: flex; // this line was already in ur code justify-content: space-between; width: 90%; }There are just details, you have done it fantastic!
Keep pushing and happy coding!
Marked as helpful
0 - peta• 110
@petaa33
Submitted
I have problems with inactive ratings not changing their background color to dark-blue once they are not active, instead they stay light-grey. Any feedback and help is welcome.
Manuel Gil• 340@ManuGil22
Posted
Hey there @peta !
What i did was to create a class called --checked. With css added there all the styles for the rating buttons when clicked. Then with js, in the eventListener for rating, removed the class --checked for all rating buttons, and added it to the one clicked.
- CSS
.rate-btn--checked { background-color: hsl(217, 12%, 63%); color: hsl(0, 0%, 100%); }- JS
rateBtns.forEach((btn) => { btn.addEventListener('click', (e) => { // removes checked class from all rating buttons rateBtns.forEach((button) => { button.classList.remove('rate-btn--checked'); button.ariaChecked = 'false'; }); // adds checked class to clicked button e.target.classList.add('rate-btn--checked'); e.target.ariaChecked = 'true'; }); });- Also if u can try to add and alternative text to the <img> tags. Its a description of the images for the screenreaders!
Well done and keep coding!
0 - Kohsey• 40
@KohseyPower
Submitted
Feedback welcome ! ^^
This is my first submition. I'm trying to learn and practice HTML and CSS. There are probably bad practises, notice me !
Manuel Gil• 340@ManuGil22
Posted
Hey Kohsey!
Im pretty new to html and css too but have some feedback:
- Im not sure which aproach is correct but i would rather use an <h1> tag instead of a <p> for the title.
- Not need to have <div> outside the paragraphs. As I told you before im pretty new too but think its just no needed. Divs are mostly used to customize and style some sections, for e.g. if you want to change the background-color of the title. In this case you are not doing anything with them so, there just extra lines.
- One of the accessibility issues you have is that every <img> tag should have an alternate text. Its used by screenreaders as a description of the image. I would do smth like alt="qr code".
- Also u have an accessibility issue that its not important but u might want to fix it. It says you should have a main landmark in ur code. That can be easily fixed by adding a <main> tag around the div with id="block". Just like this:
<main> <div id="block"> .... </div> </main>There are all details, nothing big. You made an amazing work there!
Keep pushing and happy coding!
Marked as helpful
3
