@neonitaSubmitted over 2 years ago
I would appreciate feedback on my JavaScrpit if you have the time to check it out, thanks.
James
@AuddityAll comments
- @AuddityPosted over 2 years ago
Hi Neonita,
Great job. I've never used axios but I can follow what's going on, so I think you're code is clean. I think it's cleaner than mine for this project. The only thing I noticed is you have the apiUrl twice as both a global and local variable.
2 - @Heph-zibahSubmitted over 2 years ago
Hi, this is my first Javascript project and I will love feedback on the following roadblocks:
What I found difficult while building the project? Positioning the click button in the middle. I know i am supposed to use absolute after positioning the main div as relative. However, I found it had placing the image to the middle.
Now, is there a covention on browswer use? I used Chrome and Firefox for the project but I realised they all have different output when it comes to responsiveness. Is there a cheat sheet to build a project that is responsive on all browsers? Kindly give suggestions.
Which area of my code am I u sure of? The responsiveness.
Question about best practices? Like I mentioned above, guideliines on browser responsives.
@AuddityPosted over 2 years agoHi there, good job.
Best practice for building responsive sites is to build it mobile-first. So do all your styling based on a mobile view-port. More and more people are viewing sites on mobile devices, so it's good to ensure your app or website looks best in there.
You can use dev tools and shrink the width of your screen or use the toggle device button and select a device to get an idea of what the site will look like on that device. When you start work on the larger view-ports you'll use min-width in your media queries instead of max-width.
Also, don't worry about a width smaller than 375px. The smallest cell phones have at least that wide of a viewport. Other popular view-ports are 768 (iPad), and 1024(iPad Pro). So depending on the content of your web-site you may want to use those width's as media-query break points.
Keep practicing!
Marked as helpful1 - @GitNuttsSubmitted almost 3 years ago
First time creating a project with some JavaScript in it, albeit a very small amount.
Appreciate any feedback on the structure of my code. If there's an easier way to do anything I've done, please let me know!
Thanks :)
@AuddityPosted almost 3 years agoThe JS is good.
A couple syling suggestions:
Put your share element (.share) inside your .wrapper element. The positioning of the .share element will be easier for placing it at the very bottom of the card.
For mobile view you can use both arrow images positioned in the same place. Then toggle visibility for them in the JS as well. Making it appear like it's the same button that's just changing color.
Lastly you can fix your error in the report by replacing the <button> with an anchor tag.
Cheers, keep it up, it's looking good.
Marked as helpful0 - @Scott-WilderSubmitted almost 3 years ago
I could not figure out a better solution for positioning the error message and image inside the form. This proved tricky to do when screen size changes to display that error msg and img in the same place.
#error-img { margin-left: 15rem; margin-top: -2.5rem; visibility: hidden; padding-bottom: .5rem; } #error-msg { padding-top: 2rem; padding-left: 5rem; visibility: hidden; color: red; }
@AuddityPosted almost 3 years agoYou're off to a good start. Here's some suggestions.
Your error message positioning is good. Text align it to the right and its position will match the design.
Try positioning the image absolute. Instead of using margin to move it around you can use the top and right properties. (Don't forget to set the position on the parent element to 'relative').
You have multiple elements (such as the error message & image) that have the same id. #error-msg & #error-img should be classes. This will solve about half of your HTML issues in the report.
Lastly, all input elements need a label and that label's 'for' attribute has to match the input's id. The label also has to have 'text'. But since you're using placeholder (which I did too) there's two easy options so the 'text' isn't seen. You can position it absolute and move it way offscreen or you can use the 'aria-label' attribute and it's value can replace the text content.
Keep it up and happy coding! Cheers.
0 - @outerpreneurSubmitted almost 3 years ago
Another exercise completed!
The accordion was a big challenge but it came out fine, quite happy with the results. there is an accent next to the input arrow, don't know where that comes from.
my real struggle has come with the images. I can't place that floating box horizontally, the same goes for that shadow below the graphic. I also struggle to crop that icon outside of the card.
As for the mobile version, I haven't tried very hard, got really tired after working the accordion but I will look it. I have put everything in one single column, the accordion content is protruding from the card. I also need to place those images right on top of the card.
Any suggestion will be appreciated.
Best
@AuddityPosted almost 3 years agoSorry for taking so long to respond, I just saw this.
I used a pseudo element and positioned it using absolute. However if I was doing it again I'd use the background property on the element itself (such as you have done) and use shorthand in the background property. If you're unfamiliar with the shorthand you can use properties such as background-orgin, background-position, background-size, etc. instead. You'll write less css with shorthand, but using the specific properties are easier to recognize what's happening right away.
Here's a link I like to reference when I need to remind myself of the shorthand for the background property: https://www.webfx.com/blog/web-design/background-css-shorthand/
Marked as helpful0 - @outerpreneurSubmitted almost 3 years ago
Another exercise completed!
The accordion was a big challenge but it came out fine, quite happy with the results. there is an accent next to the input arrow, don't know where that comes from.
my real struggle has come with the images. I can't place that floating box horizontally, the same goes for that shadow below the graphic. I also struggle to crop that icon outside of the card.
As for the mobile version, I haven't tried very hard, got really tired after working the accordion but I will look it. I have put everything in one single column, the accordion content is protruding from the card. I also need to place those images right on top of the card.
Any suggestion will be appreciated.
Best
@AuddityPosted almost 3 years agoGood start!
I found this challenge easier to solve by designing mobile first. There's a specific illustration used just for mobile.
First suggestion I have is switch the classes .container and .card. The 'container' will hold your 'card', and the 'card' holds the rest of the content. Also, make the first element inside the body tag a main tag instead of a div (this main tag can be your .container). This will solve one of your HTML issues in the report as well.
In managing the illustrations, try putting them in their own 'container'. I'd suggest a <figure> tag to work as a 'container' element. Put the illustration that's just a box in it's own figure element, and both of the other illustrations (mobile & desktop) in a single figure element. With the box being in it's own container it can be hidden when in mobile view and when in desktop it can be positioned aboslute within your 'card' div.
Also, that will allow you to hide the overflow of the figure element that the desktop illustration is in. Giving you the ability to crop it.
As for the shadow image, I found it easier to place that in the css as a background to the card.
Hope this helps with some of the things you want to fix! Cheers.
Marked as helpful0 - @Alejandro1709Submitted almost 3 years ago
Hello i have a question... why my media queries wont work?? thanks
@AuddityPosted almost 3 years agoThe media query works, when I shrink it to 375px the elements get stacked.
One thing I noticed is that in your css you have flex-direction set to row for the main. This isn't necessary as the default direction for flex is row
Also I noticed, under the media-query you have flex-direction set to column on both the main and the container. It only needs to be on one of these. Another thing you could do is put the class container on the main element, eliminating the need for an extra div.
Hope this helps a bit, good luck.
Marked as helpful0 - @Alejandro1709Submitted almost 3 years ago
What can i improve??
@AuddityPosted almost 3 years agoGood start.
Inside of the div with the class .review set the display to flex and align-items to flex-start, that will get them aligned in a row instead of being stacked for the desktop view.
Also, set the h1 and the <p> inside of .review to font-weight: 700
That will get it looking closer to the final desktop layout.
Marked as helpful0 - @JoshAnabaSubmitted almost 4 years ago
Feedbacks will be appreciated :)
