Tribute to SINAI - Need your feedback,thanks in advance

Tribute to SINAI - Need your feedback,thanks in advance
0

#1

here is my tribute

Some feedback would be nice… beside how can I solve the problem of ;
large gaps are forming in the text using justify on a small screen? :sweat_smile:

thanks in advance :smile:


Build a Tribute Page Questions, Discussion, and Resources (October 2017 Cohort)
#2

I like it. Its simple and works. That said I would add border radius to the smaller images so that you have a consistent look with the images. Maybe not the same radius as the main but just enough so it doesn’t appear rigid.

As for your problem with text-align: justify;. Unfortunately theres not much you can do to stop that. Its a mathmatical algorithm that makes sure that all the words on one line space out just enough to take up the entire line. As far as I know, there are two ways to solve this.

Increase width on container just enough to allow for breathing room for the words.

or

Setting word-break: break-all; to your p tag. This will make it even by letting your words split mid word. Test it out and you’ll see what I mean.

Also I would kill the margins for your paragraphs. Either that or set a consistent margin-left instead of the 10% thing. Because your using 10% instead of a flat number, some of the paragraphs are off center and don’t line up.

Other wise its looking pretty good! Keep it up! :+1:


#3

@ItsRoyal thanks …i appreciate your words …
according to the text I decided to give up on text-align: justify; … i will just keep it unjustified :sweat_smile:
i made i a radius for the images’ borders i think they are better now thanks :smile:
for the margins I tried to fix it without using fixed values … I fear it makes the page less responsive …what do you think ?


#4

Love it dude!

I would say a couple of things.
I am still going through some of the code but first off you most likely don’t need the html selector you have at the start of your css code.

Only because you already addressed the body background color in the body selector.

And you do have a few instances of very wet code throughout the css as well.
With the rows for example there are a few instances where you could clean them up and place some of the code in a catch all selector like just row for example.

Example:

row {
color: white;
}

or

row 1, row 2, row 3, row 4 etc. {
color: white;
}

and then whatever is unique in terms of your styling for the rows keep separate.

These changes are really a matter of preference but will help you in making any tweaks down the road and also anybody who might use your code or look to edit it for their own purposes as well.

Other than that man I love the creativity.
The colors are great.
I think you did an awesome job.

Keep it up!


#5

Nah. good way to maintain responsive design is to use flexbox. I could do it for you but I think you should look up flexbox yourself and learn how to use it. Really useful tool.

That said, your CSS has alot of unneeded code. #row1 - row7 is litterally the same code. You dont need 7 CSS selectors to do the same thing. Just make one id for all of them. Another thing is all the !importants in your CSS. Im assuming you did this because bootstrap was overriding your styles. You just have to make your selector more specefic. The browser will use whatever CSS has the more specefic selector so you have to be exact with your CSS when using something like bootstrap.

In most of those cases… you dont even NEED the !important tag. The CSS would work without it just fine.

Long story short. Checkout flexbox and optimize your style-sheets.


Really good flexbox tutorial.

Also just incase you take this the wrong way. Dont take this the wrong way. ^ Site is still really good for where your at which is why im pointing out the bad things so you can get better. :+1:


#6

@mohamedeliwa I like the colors you’ve chosen, the design, the images, and I agree with the comments and feedback made by @ItsRoyal and @dayashton. Great work here!

Here are my additional :slightly_smiling_face: thoughts:

Is line 4 of your HTML correct:

<div class="container.fluid"> I thought it was supposed to be <div class="container-fluid">, with a dash, but I don’t know bootstrap well enough so maybe both ways will work.

Even though it works and it’s handy, styling IDs with CSS is sometimes frowned upon. Here’s something to read about that if you’re interested: http://screwlewse.com/2010/07/dont-use-id-selectors-in-css/

What do hhead and phead stand for? I can sort of guess what you mean, but I think it’s ok to be more expressive (i.e. semantic) with your class and id names. That makes it easier, for me anyway, to read other people’s code. :slightly_smiling_face: Here’s a good article about using semantic class/id names: https://css-tricks.com/semantic-class-names/. This is mostly a matter of preference rather than an issue of functionality.

When I check your anchor element <a> and background colors in a contrast checker, I get a failing result: https://webaim.org/resources/contrastchecker/?fcolor=337AB7&bcolor=292F33. You may want to check your other colors just to be safe. I use this tool all of the time, along with http://paletton.com/ to get my colors right. Granted, I’m not a graphic designer and am still new to these tools, but so far they’ve worked well for me.

If you get a chance, run your code through this validator to clean up any HTML errors: https://validator.w3.org/nu/#textarea. I notice you have a few minor things to fix. Also, if you check the “outline” checkbox, you’ll see that you’re missing a couple of heading levels. As far as I know, the outline itself is not used too often (if at all), but having a clear heading structure helps with Search Engine Optimization (SEO) and related processes. This validator has saved me a few times.

I know FCC doesn’t discuss this too much at this level, but I try to avoid inline styles like <em> whenever possible. Here’s a good discussion about the separation of concerns if you have extra time: https://www.thoughtco.com/avoid-inline-styles-for-css-3466846.

I think this, along with the other comments, gives you a lot to think about. I echo @ItsRoyal’s comment about not taking this the wrong way. I give the feedback that I hope to receive, if that makes sense.

As an aside, I picked up a lot of these things from reading the feedback of other people’s projects. It’s interesting how many people have similar issues. You can see this feedback on other projects I’ve reviewed here. I had all of these issues (and more!) with my code at one point or another and probably will have these and other issues with my code going forward.


#7

@dayashton thanks,man I really appreciate your feedback :smile: … when I tried to comment out the html selector the all layout disrupted.
I think this is because I made a specific margins for the body element.
about my css code ,yeah I know it is a mess. I will try to rewrite my code again and improve it.
@ItsRoyal no no, I’m not taking this wrong at all, really I’m happy with your notes. besides the tutorial was great … this tool is awesome. and about my css code I will try to rewrite it again. thank you again for your notes I hope you are here always to give me your opinion :smile:
@camper thanks for your words and for those useful tools, and you are right it is “container-fluid” :smile:
any way thank you guys