@tab21Submitted over 2 years ago
Please provide feedback :)
Marvin Baga
@mjbagaAll comments
- @mjbagaPosted over 2 years ago
Hi, @tab21. Nice job on the card. It looks great.
I've checked the code and see that you're using a background image. I also suggest try doing alternate one where you're using an image tag. This might not be useful now but typically on the job, the image tags are the way to go because:
- Screen readers (assistive technology) won't be able to read background images on CSS. They can read image tags and their alt attributes.
- Sometimes when working with backend and the image source comes from them, they can't place the image unless they use a style tag and some policies like CSP prevent inline styles.
Otherwise, solution is great. Hope this helps. Happy coding.
1 - @Dany-GitHubSubmitted over 2 years ago
Any feedback is welcome , Thank you !
@mjbagaPosted over 2 years agoHi Ali. Nice work on the challenge.
Just some feedback:
- Was already pointed out on the social media icons. This usually happens when height and width doesn't match. You can try setting an explicit height and width, maybe in pixels. Or you can define just the width or height and use
aspect-ratio: 1/1; - I'm on a larger screen and the social media icons are cut off, almost by half and can't scroll down. Maybe doesn't need the
overflow: hiddenon body. - On your button, maybe you can add a small transition effect,
transition: color 300ms ease-in;. These are micro-interactions that improve user experience.
Hope this helps! Happy coding!
Marked as helpful0 - Was already pointed out on the social media icons. This usually happens when height and width doesn't match. You can try setting an explicit height and width, maybe in pixels. Or you can define just the width or height and use
- @emjogaleSubmitted over 2 years ago
I have had a go at making the slider accessible but would like some feedback on how to improve this. I also struggled with the positioning of the background images - any advice on this would also be welcome!
@mjbagaPosted over 2 years agoHi, Emma. Good work on the slider!
Nothing much to say except maybe a few ideas on transition of the slides. Small animations just make the user experience better. Some things you can try:
- Slide animation, usually you declare a long width so that you can place your slides side by side and use overflow-hidden to hide that slide. Then you can play with transform translate rule and use transition. Or even use animate CSS rules.
- Fade in, fade-out, is much easier, you just stack the slides up and fade in the the new slide.
Oh, lastly, maybe a better hover effect than shadow on the buttons. Maybe background change instead.
Hope this helps! Happy coding.
Marked as helpful0 - @topspinppySubmitted over 2 years ago
Please Feedback I learned scss first time, I'm not sure about scss best practice :D
@mjbagaPosted over 2 years agoHi @topspinppy. Good job on the challenge.
I've checked your code and have a few feedback:
- I would recommend to separate your media queries in big group chunks, instead of repeating the media query includes on a few declaration blocks. It's something I also used to do, but when you check your compiled styles.css, you'll see the same media queries were repeated like 16 times on the file. It wouldn't matter much on very small projects but quite significant or large ones, even saving a few kilobytes.
- You can check out mobile-first development. It helped me out a lot in coding for responsive design. Basically it's coding for mobile first and adding media queries as you go up in screen size. So you'll be using your min-width mixins instead of max-width.
That's it. Hope this helps! Happy coding!
0 - @NiezzxSubmitted over 2 years ago
Here is a problem ,When shrinking the label, box - image is hard too adjust. need some suggestions.
@mjbagaPosted over 2 years agoHi, Niezzx! Good job on the challenge.
I've checked your code out and here's some feedback:
- The container is expanding because of the hidden paragraph content being shown. Notice that it doesn't expand when the paragraph isn't as long as 2 lines. Try transferring that hidden paragraph inside the question-bar div so that it will follow the width of its parent container and you won't have to us percent width on the paragraph.
- I checked your JS. For adding event listeners to querySelectorAll, you don't need to loop through each item. I get that you're trying to access the ID, but you can just do it by simply traversing the DOM.
- Check out this article https://medium.com/codex/how-to-traverse-the-dom-in-javascript-7fece4a7751c for traversing DOM. Basically if you have queried an element, like with querySelectorAll, you can use that as a basepoint to either go up the DOM to access parents, or go down the DOM to access children.
Hope this helps! Happy coding!
Marked as helpful0 - @devbevSubmitted over 2 years ago
Hello all,
Please check out my work and kindly provide any possible advice or suggestions.
Thank You.
@mjbagaPosted over 2 years agoHi Devbev. Good job on the challenge.
Just some feedback, just very small things:
- Just semantic HTML, suggest to use
<ul></ul>for lists, items that are related with each other. To divide stuff into sections for layout, use divs instead. - Font sizes are smaller and the colors for Equilibrium and Jules Wyvern are different from design.
That's it! Otherwise, nice work. Hope this helps.
Marked as helpful0 - Just semantic HTML, suggest to use
- @jesuisbienbienSubmitted over 2 years ago
I'm getting more comfortable using grid. Also I spent time updating the readme.md file and I'm proud of it. I'd love to receive feedbacks on anything.
Some specific questions I have are: 1- any other way to make this design work? using grid or not using grid? 2- what would you do differently for the box-shadow, that makes it look closer to the design? 3- how do you like the active state that I added for the cards? :D
Happy coding!
@mjbagaPosted over 2 years agoHi, Nguyen. Good job on the challenge.
Your questions:
- I'd use grid as well, much easier with grid-template-columns/rows or grid-template-areas.
- I suggest maybe add a transition effect on your hover for nicer micro-interactions. Something like
transition: box-shadow 500ms ease-in-out;. Also maybe less shadow spread so it doesn't cover up any other elements (unless intended).
Hope this helps. Happy coding!
Marked as helpful0 - @JessicaDubemSubmitted over 2 years ago
Hello everyone. I used react for this project. The website is fully responsive, but I had difficulty resetting the form in react. I would appreciate some feedback on how the code is written and any suggestions. Thank you all.
@mjbagaPosted over 2 years agoHi, Jessica. Good work on the challenge.
I've checked your code and here's some feedback:
- Try adding a container for max-width on the breakpoints so that the site won't be very stretchy, especially when reaching very wide screens.
- For file organization, maybe try breaking down your header and footer on their own components.
- You can look into mobile-first styling for handling responsiveness. It's a concept where you style out your mobile CSS rules then add media queries as you increase screen size like for tablet or desktop.
- For this last one, just a personal preference (you can ignore haha), I'd say either just go full Sass or full utilities library, but I'd go with Tailwind as it's really more flexible than bootstrap, especially with responsiveness.
That's it. I've also done Designo, you can check it out. Hope this helps and happy coding!
Marked as helpful1 - @MonicaDalostoSubmitted over 2 years ago
Hey Folks, I've just finished my third project, and I'd really appreciate any feedback to help me improve my code. I didn't use bootstrap or other tools like that. I tried to use the concept of Utility First in CSS, but I got confused and had problems with some classes, especially on the @media screen part... So I restarted the code to the normal concept. How can I improve or make my code more advanced... As a newbie, I am open to any other suggestions. Thanks in advance!!!
@mjbagaPosted over 2 years agoHi, Monica. Card looks awesome. Nice job.
Tip for media queries, maybe you can look into mobile-first styling. So it's concept where you set your css rules for mobile first, then when there's rules that change as you go higher like tablet and desktop, you do your media queries then. Really helped me a lot in doing responsive designs.
Marked as helpful1 - @WayneHaworthSubmitted over 2 years ago
Different widths between the image and the text below. Interested to see how others handle it elegantly. I solved this by restricting the width of the text area to 90%, but curious to see how others have done it.
@mjbagaPosted over 2 years agoHi, Wayne. Great job. The good thing about CSS is there's lots of ways to approach things. I would have probably added padding-inline on your text-container, but reducing width also works well.
Some feedback, for the image, I tend to not be explicit on the width in pixels, especially when parent width is already defined, just have the image try to follow the parent width with 100%.
Anyway, nice job. Happy coding.
Marked as helpful1 - @fellipemndsSubmitted over 2 years ago
The property "margin: auto" only centralizes it horizontally, but I would like to do it vertically as well, so, how can I centralize the #container div, both horizontally and vertically?
@mjbagaPosted over 2 years agoHi, Mateus. You can centralize elements inside a grid or flex vertically and horizontally using "place-items: center;". It's the short version of "justify-items: center;" and "align-items: center;".
For flex, depends on which direction if it's column or row, justify-items or align-items.
For non-grid or non-flex, try "vertical-align:" sub or middle, etc. You'd have to manually check.
Last would be just adding padding on inline elements.
Marked as helpful1
