@TrumanchhoSubmitted over 1 year ago
My 6th challenge. This one was a little more tricky. My CSS got a little messy especially when creating the navigation.
- I learned how to create a nav-bar
- learned how to animate on hover states and button clicks
P
Alan Ibarra
@Ibarra11All comments
- @Ibarra11Posted over 1 year ago
Overall, your solution to the challenge looked great. One thing I noticed in the article list section is that you are using margin-right: 10px on each item. You could add gap: 10px to the where you define display: flex. On desktop, the last article item is not aligned to the right because it has margin-right: 10px. You could fix that by not adding 10px to the last item, but gap handles that for you it only adds space between items not at the ends. Also, in the article list you are transitioning from display: block to display: flex where you can just do something like this
display: flex; flex-direction: column; gap: 30px; @ media(desktop size){ flex-direction: row; gap: 10px } ```;Marked as helpful1 - @kevinx9000Submitted over 1 year ago
I discovered I had to nest more <div>s than expected in order to get the desired layout through Flexbox. It felt like my HTML markup got more cluttered than it should that way, but it seemed to be the only way I could accomplish it. The hover overlay effect on the main image would have been a real challenge, but luckily a completed project from a Udemy course had a similar design, so I went back to that to refresh my memory and was able to pull it off. I didn't seem to need to create a mobile breakpoint because the size of the card was small enough to retain the same look on mobile.
@Ibarra11Posted over 1 year agoI think overall you did a good job on structuring your HTML and didn't see no issue with the divs. One thing I did notice is that on your .avatar-section class you don't need to include flex-direction: row and justify-content: flex-start. Those are the default values when you use display: flex. Also, there are some situations where you use padding or margin to distance flex children where you could use gap property.
Marked as helpful0 - @Ajwahib95Submitted about 2 years ago
I decided to undertake this challenge on Reactjs to practice state manipulation and other react basics, if there's any feedback on best-practices or more optimal ways to code this challenge, it would be much appreciated.
@Ibarra11Posted about 2 years agoI noticed that you have selectedRating as a variable outside of the component. I think it would be more appropriate to be used as a state value in your App component. Then you can just make a handler in the app component that takes a rating value and updates the rating state variable. Then you can just pass the rating value to RatingCard as a prop and the handler for it as well. This will eliminate the need to keep in isActive state in the rating card component, you can just transfer that logic into the handler. Also, you ratingCard component you have a lot of repetition with the rating values. You can instead just map over an array of ratings.
define outside of your component const ratings = [1,2,3,4,5]; {ratings.map(rating =>{ return (<div key={rating} className={props.selectedRating === rating apply the conditional styles} onClick={() => updateRating(rating)}}> <span>{rating} </div>) })}0 - @ghintemaSubmitted about 2 years ago
I used aria-expand on the hamburger menu-button to improve accessibility. What do you think of it? I would like to get some advices on how to make the card-preview (pop-up) accessible for screenreaders. I didn't get that.
I also build a fully functional slider with auto-play and gallery-navigation. But due to that I waved the extra modal meant in the design... Feel free to comment on my work! :)
@Ibarra11Posted about 2 years agoOne thing I noticed is that the shopping cart button is not accessible via keyboard. It only works if you click on it, but it should also respond to the enter key that way someone that uses a keyboard can open it. When the modal is opened, it needs to trap focus in there meaning that a keyboard user wouldn't be able to interact with the things outside of it. And, when the modal is closed you should return focus back to the button that triggered the modal. For example, if the add to card button cart triggered the modal when the modal is closed the focus needs to be returned back the add to cart button. There is a lot of accessibility concerns that comes with modals, so in most cases people tend to use modals that other people have built and are reliable.
Also, on the mobile navigation if you press tab to cycle through the interactive elements it takes about 6 tabs to reach the hamburger menu. I can still interact with the modal even thought you pushed it off the screen. One thing you can do is to use display: hidden when the user hasn't clicked the hamburger menu that way it is no longer on the screen and not interactive. However, I'm not sure how this will effect your animation. Another possible solution, would be to add tabindex="-1" on the links when the menu is not open and remove it when the menu opens. This is just an idea, I'm not sure that it will work.
0 - @DavidBurgess1984Submitted about 2 years ago
I've attempted to build this using SASS. Can someone help me fix the background images. The deep purple lozenge on the left never seems to align right, and the right faded lozenge overshoots the bottom of the page. I have tried to use
position:absolutebut can't seem to get it right. I'd appreciate any help or comments.@Ibarra11Posted about 2 years agoOne way you can fix the issue of the right faded lozange overflowing the bottom of the page is by putting overflow:hidden on the body. That will crop off the lozange when it overflows and also will allow you to position the bottom right lozange how you want. You can move it to the right to make it smaller or however you want it to loo like.
0 - @Semi-SquareSubmitted over 2 years ago
My first attempt at an intermediate level design. Once again, this is only for practice. I feel like there are places I could have done better on the responsive end. Please feel free to comment and give your feedback!!
@Ibarra11Posted over 2 years agoOne improvement you can make is to make the images in the articles section responsive. They start getting distorted when you start scaling down the viewport. One easy thing you can do to make your images more responsive is to add object-fit: cover. Now your images will scale better when the viewport changes.
1 - @TiyyoSubmitted over 2 years ago
i didn't succed to do the overlay effect on this challenge
@Ibarra11Posted over 2 years agoYou had the right idea with the overlay nesting it within the picture. After that you have to set the overlay to absolute and you want it to fill the entire picture container, so you have to give it 100% width and height. At this point the overlay is sitting on top of the img and once you give it a background it will hide the image. You have two options you can set the overlay to display: none or set its opacity to 0 and when the user hovers over the picture you set the overlay class to either display: block or opacity 1. In my opinion, I think the opacity choice is better, but you can choose which make ever makes sense to you. I'm going to include a code-sandbox if you want to take a look at how I implemented it - Sandbox. https://codesandbox.io/s/objective-galois-chtm1u?file=/index.html
0 - @RomWeb3Submitted over 2 years ago
I'm not sure about my grid template, is it good? To place the grid I used margin-top, is there a better way to do it? Thanks :)
@Ibarra11Posted over 2 years agoYour solution is not bad, but I think it can be improved using some of grids features. The approach I took is for the first and third grid items to span 2 rows. After that I just used align-self: center on them to center them within the two rows. Now you can remove margin from the karma card it aligns itself perfectly.
.box1, .box4{ grid-row: span 2; align-self: center; }Marked as helpful0 - @milandsharmaSubmitted over 2 years ago
GitHub is not showing my chart bar cost on hover. I don`t know why this is happening . please friends help me.
@Ibarra11Posted over 2 years agoThe issue with the data not showing on hover is because you are making a fetch to a url that does not exist. What is happening is that when you do fetch('/data.json'), it makes a request to "https://milandsharma.github.io/data.json" and there is no server or something to return data back to you. Also, I think fetch is not appropriate in this situation since you have the data already, instead you can just import the data and then use the data. It would look something like this inside of your script.js.
import data from "./data.json";Marked as helpful1 - @PinkUvSubmitted over 2 years ago
I ran into two issues I need some help fixing if possible.
First, the active hover for the main image is incredibly buggy, I had to end up eyeballing the measurements because they seem to be constantly scaling based on viewport size. However, this results in a warped hover whenever your display doesn't match my native monitor size; I could use some help finding out why it keeps resizing and how to fix it.
Secondly, when it comes to the icons for the price and time they aren't centered along with the text. I positioned both using ::before relative to their classes but I can't seem to get them to move after that. Is there a way to fix their position? or do I have to scrap using ::before and try something else?
Other general code critique and criticism is welcome, thanks in advance!
@Ibarra11Posted over 2 years agoThe main issue with the hover overlay is that it is not constrained to the box that contains the image. In this case, you want want to set the article with class of main-image to relative now the overlay which has the property of absolute will be positioned relative to the article element. With that being set you can set the width of the overlay to 100%, which will be the width of the article element and give it a height of 100% and will give it the height of the article element. I also moved the margin from the image to the article element and removed padding as well. Linked is a code sandbox if you want to take a look: Code Sandbox
Marked as helpful2 - @kein-1Submitted over 2 years ago
Couldn't figure out how to partially hide the images like shown
@Ibarra11Posted over 2 years agoMy first thought when I seen your issue is to resolve it using the z-index. Basically, z-index controls which element appears on top when you have two or more competing elements. For example, you have the tab and the item container overlapping each other. All you have to do is give the item container a higher z-index and it will appear on top and give the illusion that the images are cropped off. But in order to use z-index, you have to create a stacking context. In this case, you can create one using flex-box. So, all I did was making item-container a flex parent and the images were partially hidden. After that whatever element you want to appear on top all you have to do is give it a higher z-index value. This will work without providing a z-index value because it will defer to using the DOM order. In other words, if you have two elements with the same z-index value the one that comes later in your HTML comes out on top. In this case, .item-container comes after .tab div, therefore coming out on top. If you wanted the tab to come out on top you would just give it a higher z-index value. Another way you could have solved this is by setting a height on the .tab div and using overflow: hidden on it so when the image spills over the tab, it would get cropped off. If you want to learn more about z-index this is a great article: https://www.joshwcomeau.com/css/stacking-contexts/.
.item-container{ display: flex; flex-direction: column; }Marked as helpful0 - @tbensheimerSubmitted over 2 years ago
Hey everyone I completed another Junior Project with an API. This project was really fun to build as I have learned a way to toggle between themes. I also utilized a different solution to fetch the API than the previous advice generator project. Another concept I learned was being able to use the slice method in JavaScript to eliminate the unnecessary information that was retrieved from the API (specifically the date the users have joined). Please let me know if there is any way I could improve my code! Thank you in advance.
@Ibarra11Posted over 2 years agoIm not sure if you notice, but your app overflows in both the X and Y direction. As a result, there is a horizontal scrollbar and a vertical scrollbar. The issue is that the browser adds a default 8px margin to the body. I would suggest to include a CSS reset stylesheet, so you override the default browser styles and your app looks the same across different browsers.
Another thing I noted is your conditional logic. It's not incorrect ,but I might be more concise to use ternary operators. For Example,
bio.innerHTML = data.bio ? data.bio : 'this profile has no bio'; twitter.innerHTML = `<span class="icon"><img src="assets/icon-twitter.svg" alt="twitter icon" /></span> ${data.twitter ? data.twitter : 'Not Available'}`For your classes, you are adding them though the classList.add but I think you can just do it straight from the HTML. It would be something like this
const twitterClass = `icon${data.twitter ? "" : " notAvailable"}`; twitter.innerHTML = `<span class=${twitterClass}><img src="assets/icon-twitter.svg" alt="twitter icon" /></span> ${data.twitter ? data.twitter : 'Not Available'}`Marked as helpful0
