@MahdiAljazairi
@steeven509
Submitted
Hello everyone 😎 I hope everything is okay for everyone 😃 If you have a few minutes to give me to check my code and my site will be great because you are great people I love you don't hesitate to give your criticism
@MahdiAljazairi
Posted
@steeven509 Now have I figured out why this isn't working! And tbh, it's kinda embarassing 😅
The :hover pseudo-class must be put after .card__img, not ::before! Just like this:
.card__img:hover::before { ... }
Why? Because, according to the spec, pseudo-elements (like ::before) can't have pseudo-classes (like :hover). So you have to apply :hover to an actual element (like .card__img) for it to work.
For that reason, the selectors in main.scss should be arranged like this:
.card {
...
&__img {
...
&::before {
...
}
&:hover::before {
...
}
}
/* rest of the selectors.. */
}
Marked as helpful
@steeven509
Submitted
Hello everyone 😎 I hope everything is okay for everyone 😃 If you have a few minutes to give me to check my code and my site will be great because you are great people I love you don't hesitate to give your criticism
@MahdiAljazairi
Posted
Hi There! I have spotted some flaws in main.scss:
In line 62, the ampersand was placed incorrectly:
:hover &{
Then the output in style.css was:
:hover .card__img::before {
Which caused the ::before to appear wherever you hover on the page.
It's no big problem, though. Just put the ampersand before :hover and thats it!
.ethereum should be bold.In line 94, there is .card__data-info instead of just &-info. For which the output in style.css was:
.card__data .card__data-info {
...
.card__data .card__data-info .ethereum,
.card__data .card__data-info .clock {
...
/* and so on.. */
This has added an unnecessary layer of specificity. It won't break the webpage. But in terms of reusability, it might make changing the styles of this part a bit more difficult further in the code --say, in a media query, for instance. Not to mention that this actually kills the purpose of using BEM in the first place!
Now, as well as the above, I have some suggestions for overall improvement:
Since you're already using Sass, why not make use of Sass variables ($xxxx), instead of CSS custom properties (--xxxx)? As long as you don't intend to alter the values of these variables via Javascript, Sass variables are the best option because they are compiled into their actual values in the output file.
Also, CSS custom properties are not supported in Internet Explorer. So if you want users that --are forced to-- use IE to be able to view your website, this is another reason to prefer Sass variables.
There is an element called <hr>, short for 'Horizontal Rule'. It is used to make a break in the flow of content. Its effect isn't only visual --as you can always change how it looks like. It also gives assistive technology a hint to announce to their users this break in the content.
This element could be used in this project for the line that seperates the author's info from the rest of the component.
That's about it! I hope it was helpful. And as they say: Happy Coding!
Marked as helpful
@kazerpanol
Submitted
@MahdiAljazairi
Posted
Hi There! I have some suggestions for you! I hope they will be useful.
Try to avoid using heading tags <h1> ... <h6> only to adjust font size. Heading tags give assistive technology information about the hierarchy of the document, e.g. a section with a <h1> heading can contain another section with a <h2> heading and so on.
To specify font size without giving any special role to text, use CSS.
**HTML**
<div class="advice-id">ADVICE #117<div>
**CSS**
.advice-id {
font-size: 0.8em;
}
Turn your contents' wrapping element into <main> and your attribution element into <footer>.
<body>
<main class="flex">
...
</main>
<footer class="attribution">
...
</footer>
</body>
This way you can tell users of assistive technology which part of the page is important <main>, and which is less important <footer>. Also, the accessibility report will stop yelling at you to put all of your page's content in landmarks 🙂
Use an actual <input> or <button> tag to make an "offically" interactive element. Such elements can recieve focus by pressing the Tab key, and are announced by assistive technology to the user to be interacted with.
<button type="button" class="circle-btn">
<img class="dice" src="images/icon-dice.svg" alt="dice-image">
</button>
You can use <q> tags instead of hardcoded quoting marks. These tags insert appropriate quoting marks independently of how text inside them change. This means you can put them in the HTML and play with text without worrying about adding quote mark each time you change it.
**HTML**
<p><q>It is easy ... taking action</q></p>
**Javascript**
function renderAdvice(data){
const numAdvice = document.querySelector("h4")
const textAdvice = document.querySelector("p > q")
numAdvice.innerText = `ADVICE #${data.id}`
textAdvice.innerText = `${data.advice}` // No need for quotes here
}
This is it! May this can help. And as they say: Happy Coding!
Marked as helpful
@kowsirahmed
Submitted
I faced a problem while absolute positioning the overlay above the ethereum(card main image). It shows some empty space at the bottom of the image.
This is solved when I thought about the spacing for element(e.g. CSS box model). From inspection I made sure there is no margin or padding creating the problem. Then I thought about the content size in the box model. I knew text content takes an height equal to the line height for each line. As images are inline elements, it was following the rule for text contents of line-height. So, I just removed the line height in it's container.
@MahdiAljazairi
Posted
Hi there! I guess there is a simpler solution to the problem you have mentioned. Just make images into block elements! This also avoids issues you might have if the container had actual text as well as images.
Besides that.. Good job you did 👍
Marked as helpful
@NathanMartinez
Submitted
Any feedback or suggestions are greatly appreciated!
@MahdiAljazairi
Posted
Pretty nice work imo! The only thing is that you forgot to style the "Change" link. That's it.
@Konstantinchrist
Submitted
Hi guys, I hope you are doing great. I just have one question: Why did my border-radius on the main not work?
And if you come across any issues in my code please let me know.
Thanks a lot to everyone Konstantin
@MahdiAljazairi
Posted
I just realized something that confused me for a bit. When you submitted your solution, you put a link to a commit instead of the link of the repository itself.
Click the three dots to the right of the name of the solution and choose 'Edit solution'. Then change the Repository URL field to (https://github.com/Konstantinchrist/3-col-responsive-site), and click Update Solution.
It's not that big of a problem, but it better be avoided in the future.
@Konstantinchrist
Submitted
Hi guys, I hope you are doing great. I just have one question: Why did my border-radius on the main not work?
And if you come across any issues in my code please let me know.
Thanks a lot to everyone Konstantin
@MahdiAljazairi
Posted
The corners of the divs inside of main are just flowing outside. The most straightforward solution is adding "overflow: hidden;" to main inorder to hide anything that overflow out of it.
Also, I would suggest putting the styles in an external file, rather than in a style tag. This is better both for readability and maintainability.
EDIT: Each property that transition applies to should be specified separately in a comma-separated list:
button {
transition: color padding border 150ms; /* Wrong */
transition: color 150ms, padding 150ms, border 150ms; /* Correct */
}