@3eze3Submitted over 1 year ago
Hello community, this is my contribution to this challenge. As for the coding part, I want to orient it to POO, since it is easier for me. I hope for some feedback to improve and thanks , happy coding.
Tryt4n
@Tryt4nAll comments
- @Tryt4nPosted over 1 year ago
Because you declaring width and height as a percentage, when the content is bigger then it's overlapping or when the screen size is narrow and tall it's just look not nice. You should avoid declaring width and height based on percentages values to avoid those kinds of situations. They are very problematic in terms of responsiveness. Just declare a constant value or use
min-width,max-width, orminmaxvalues.Your email validation checking only for gmail, hotmail and yahoo mails. Instead of doing some things with regex on your own there are easier way to do this. Just google out regex for email or use library like email-validator.
You can also consider using a
<dialog>element for your confirmation element. It's way better and way more accessible.Tip: In your case if you want to center element in the page the easiest way is to use
dispaly: gridandplace-content: centerorplace-items: centeron your container element so in this case on<main>element. It's easier than declaringtopandleftwith some values and then doing the math.0 - @3eze3Submitted over 1 year ago
Hello community, I have solved another exercise, applying little logic with js, and more design in the part of css that complicated me a little. How can I make the text to be above the image? I tried to play with the positions, but it gives me a lot of problems with the heights, that is an inconvenience that I had. Please if you have any improvement for my code, design, or accessibility, I would be very grateful to know in the comments. Happy coding. 🍔
@Tryt4nPosted over 1 year agoYour path for
::beforepseudo element inblockquoteis wrong. Personally I think it would be better use::beforeor::afteron your personimginstead of adding anotherimgelement which has only a decorative function.Also would be better if you would add your buttons in
imgsection. Would be easier to place them. Currently they always have different placement. Instead of that it they would be inimgcontainer you could position them absolutely to that container then addbottom: 0;left: 0;andtranslate: 100% 50%;and it would be exactly the same as in the project.Also because you use
topandlefton your buttons container with such a massive number (40remand60rem) it's completely mess with your media query. Below 1100px your buttons overlapping yourbody.Your media query for mobile should start below around 1024px in my opinion. Even when I disabled
60remforleftfor your buttons theimgelement is getting started to be very small.And some typo:
- your both buttons have the same
aria-label
Marked as helpful1 - your both buttons have the same
- @3eze3Submitted over 1 year ago
Hello community, I have finished this project and I really like the design and I added a small animation for the social links, if you have some improvements for my code, or about the accessibility, you can tell me in the comments, really help me to improve. Thanks and Happy coding. 🍕
@Tryt4nPosted over 1 year agoBetween width of 480px to around 700px on your social link icons narrow because you use
width: 30%on.footer__social. Also for<form>element you could add somepadding-inlinebecause below 580px elements inside touch the edges and personally I would addtext-wrap: nowrap;on yourbuttonbecause between 480-570px text is wrapping. For image you could change it so that it is always the same width as theform.For accessibility:
- text "soon" in your
h1could not be visible for assestive technologies. Instead of that you could wrap it instrongorbor just leave it as it is and add<span class="visually-hiden>soon</span> labelis empty. You could addspanwithvisually-hidenclass and some text like "email"- add
aria-errormessage="some_id"to yourinput - to you warning text add this
idandaria-live="assertive" - © symbol is the same as
© ;but without space between letter "y" and sign ";" - you could add some description for your footer navigation like
aria-label="social links navigation"because in real website it would be probably just a component and there would be anothernavelements
Personally I would change
spanforpwith your text "Subscribe and get notified.pis for paragraph andspanis just likedivbut it hasdisplay: inline;Marked as helpful1 - text "soon" in your
- @3eze3Submitted over 1 year ago
Hello community, I really did not encounter any problems in this project because the interaction really relied on css rather than js because it is just a set of classess with the components, I really liked it, it was something fun to do, and if you have any improvement for my code, both in the design, you can let me know in the comments, Happy coding.
@Tryt4nPosted over 1 year agoYou can add
<time datetime="">element instead of<span>. For button you can addaria-haspopup="true".Also your media queries in my opinion are set wrong. About 860px width and below it's overflowing. If popup is open overflow starts around 940px.
Marked as helpful1 - @Lo-DeckSubmitted over 1 year ago
Hi, I checked HTML and CSS with the W3C validators.
I tested this website with Chrome and Firefox, with Chrome I haven't had any issues. And with Firefox I have some :
-
the"76 of 100" are not centered I need to refresh the page. ** HTML**: line17''' <div class="mainscore"> <p>76<br> of 100</p> </div> ''' CSS: line87 '''.mainscore p{ margin:0; position: absolute; top: 2rem; color: hsl(241, 100%, 79%); }'''
-
When I reduce the size under 375px in mobile desktop(with browser tools), the website is not reponsive(but work well with Chrome and Edge). I tried to modificate some values like width in max-width but always in worse issues.
Thanks for your feedback, and your sharings.
@Tryt4nPosted over 1 year agojust use:
.mainscore { display: grid; place-items: center; }instead of what you're doing. I think it's the easiest way to center, just two lines of code.
Marked as helpful0 -
- @3eze3Submitted over 1 year ago
Hello community, this project was complicated in the Js part because I feel that the code is not quite right, but the functionality is correct, and as for the design, if you can give me suggestions of what I can improve in my Css, I would appreciate it very much, and also my Js code, and html, I think it is correct and semantic, I did not use the details or summary tags, because I wanted a more personal design. And well as always you can tell me anything about my project and Happy coding. 🧡
@Tryt4nPosted over 1 year agoFor accessibility you can add
role="regionfor everyaccordion__boxbecause all elements inside are related to each other. Then addaria-expanded="false"for everyaccordion__headand handle change state with JS when it's expanded/collapsed. Also for every<p class="accordion__description">addidand then for corresponding<div class="accordion__head">add attributearia-controls="your_id".In your case user cannot use keyboard to navigate so there are two options:
- change
div class="accordion-head"forbutton - add
role=buttonandtabindex="0"to<div class="accordion__head">
Marked as helpful1 - change
- @HK273Submitted over 1 year ago@Tryt4nPosted over 1 year ago
You've got some problem with fetching due to cors. Probably due to by IPify API. I had the same but used a different API
And also you are using React. None of your images like marker are working. All your resources like images should be in public folder and then you acces them by
img src="/example-picture"Marked as helpful0 - @AbdElmalik100Submitted over 1 year ago
- @3eze3Submitted over 1 year ago
Hello everyone, I had a problem with the fonts, well anyway I think I got a good result, and if you have any comments to improve my accessibility and my css code, I would be very grateful.
@Tryt4nPosted over 1 year agoYou should wrap everything in
main. If you decide that the one content is more important than the other you can wrap this/those element/s inaside.asideelement doesn't have to be literally aside. It's just less important content on the page.Also you skipped heading level in newsletter. There should be
h3instead ofh4. If you want write semantic HTML with accesibility you should use some evaluation tool. I personally use wave. If you want check it out.It would be good to use
afor thing like phone number or email. You useafor email but it should look like this:a href="mailto:[email protected]". Same with phone number:a href=tel:123456789. Change this and then click on those elements and you'll see why it should be like that.What do you mean that you had problem with fonts?
0 - @3eze3Submitted over 1 year ago
In this project is my first time working with Sass and maybe I have many errors or things that I could improve, so if you can tell me I would be really grateful, I wanted to complicate some things that I could have done with css but I wanted to force a little more Scss since I'm learning it. And finally the project I managed to do well in my opinion. Happy coding ❤
@Tryt4nPosted over 1 year agoIn overall it's well written. BEM class architecture is good but folder structure could be better.
src folder isn't asset
In assets folder should be images, fonts, etc.
For colors you could make new colors file
In base folder you could split the base file into several different files. For example base, font-face, typography, helpers
If you would have helpers file you could put there
visually-hiddenclass and use it in yourh1instead ofcard__title--hiddenbecause it's not modifier. You are just visually hiding this element from the user not modifying it.Marked as helpful1 - @kwngptrlSubmitted over 1 year ago
Hi, this is my take on this challenge. Feedback is welcome. While the navbar is working, I'm not fully satisfied with it. It doesn't seem to be obeying grid rules in some cases, like there's quite a bit of padding when on fullscreen mode.
@Tryt4nPosted over 1 year agoIt's because you gave
min-height: 100vh;for your body. Change it for display of flex, flex direction of column and justify-content of center. Then deletemargin-topfrom your footer and it would be done.Nice trick for debugging CSS:
* { background: hsla(100, 50%,50%, .2); }You can use something like this. It gives opaque background to everything on your page. So the backgrounds of the elements will overlap each other. Thanks to that you will be able to see where the problem is placed. Before you edit your solution just check it out. Also if you have some pseudo elements
::beforeor::afteralso give them the background.*::beforeand*::after0 - @alaykabirSubmitted over 1 year ago
- I found it difficult to integrate the API.
- Still have to learn about async and await.
- I am unsure of the javascript code for "JSON", "fetch" and "then" as I had to take help from the internet.
- How can I improve my css responsiveness?
@Tryt4nPosted over 1 year agoFirstly for your whole element you gave
max-width: 25vwthat's mean if you have mobile with width of for example 400px the element width would be 100px. You could do something like this:min-width: 25vw;max-width: 500px;That's mean width would be 25vw but no more than 500px. Of course you can change those values.
Another thing add to your
imglinemargin-inline: auto;because it's not centered. That's mean add the same amount of margin in the left and the right.Last thing add
text-align: center;to your whole element so the text content would be centered.Marked as helpful1
