@ChrisEski
Posted
Hello saira, first of all good job over there! I'm goint to list my remarks below:
-
As for the HTML part, i think it's a better practice to use classes instead of ids. By using ids i believe you might mess with specificity, especially in bigger projects.
-
About the card div, setting the height in 'vh' units makes it to change the card's height as the screen size changes, i.e. for small/normal size smartphones it's probably goint to be ok, but as the screen gets taller and bigger, such as a desktop, you notice that the card height increases leaving the content sticked in their initial position. You want the card's height to stay the same. Maybe you want the width to change with the viewport, but in this project the change is not that big. Plus there is some content overflow as the card height gets smaller than the content height
-
In my opinion, this card seems to be like a preview card of an article or something, which makes me believe that it would be more relevant to make the image and the title as links, that lead to that specific article or a further info section. Saying this, the cursor should be a pointer on hover to make it clearer that these elements have some functionality. Same with the creator's name, which should probably be a link that takes you to his/her profile or show info about him/her.
-
Now this is a personal preference, but i think that it makes for a better experience if you give the hover effect a smoother transition.
-
Last, since you are working on scss (which i am starting to love), consider dividing your style code in partials and use some more of the nesting thing, cause this is where sass really shines! It will make your code cleaner, more organized and easier to refactor/debug in the future.
These were my humble notes on your project. Hope i helped just a liitle bit there... :)
Marked as helpful
@saira512dev
Posted
@ChrisEski A little???U helped A LOT!!!Thanks A TONN!!!I have worked on all of them.Super happy about all your comments.Also i do have an issue.Now that i have fixed height,if u tilt your phone and watch the site,you will see that a small part on top cant be seen.I mean you cant scroll.Any idea why that happens and how to fix that?Thanks again :)
@ChrisEski
Posted
@saira512dev
Oh soo much better already, i'm super happy!!
Well, about your final problem, here is my solution. Simple and i use this approach really often when i want my main thing centered in the screen.
Just in words:
-
You don't want to add so much style to the
body. Usually i apply the resets onhtmland leave thebodyintact. -
Wrap your whole
mainelement into adiv.containerand apply ALL your width/height stuff on that. There is where you center everything with flexbox. -
In the main element, you just set the card's
widthandmargin. You don't need anyheightexplicity set, because the card will grow and shrink following the content of the card.
And here is code part:
HTML
<body>
<div class="container">
<main>
<div class="card">
<div class="wrapper">
...
...
...
<main/>
<div/>
</body>
CSS
body {
font-family: 'Outfit', sans-serif;
}
Display flex gets you the same exact resault in centering, except so much easier. Setting the container's min-height: 100vh is the key to center the card vertically!! (A nice trick is to set almost anywhere a border, so that you can clearly see in the browser how your elements act and their real sizes). Here is where you apply flex, and center everything in your screen:
.container {
background: var(--Body-bg);
border: 1px solid white;
width: 100%;
min-height: 100vh;
display: flex;
justify-content: center;
align-items: center;
}
At least when i did that in my end, setting the main's margin top/bottom solved the problem that you mentioned. In portrait mode, i could scroll down and clearly see the top of the card:
main {
background: var(--Main-bg);
width: 345px;
margin: 65px 25px;
border-radius: 15px 15px 15px 15px;
}
One last thing that i noticed, after you changed some id into class, you forgot to change the hover on wrapper state so the overlay is't visible. This is how this should be:
.image-container {
...
...
opacity: 0;
transition: opacity 0.3s ease-in-out;
...
...
}
.wrapper:hover .image-container {
opacity: 1;
}
Well that was all! When you apply the changes tell me if any of these didn't work! Fell free to take a look at my code on this project to compare things :)
Since I'm a beginner myself, if somebody else finds a mistake in my approach should correct me. I don't like to give false feedback :)
Marked as helpful
@saira512dev
Posted
@ChrisEski wow!!!U r seriously a life saver.I did hear that div can be placed center this way but did not know that it wil help in portrait mode :).Thank you again
