Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NW6|Bakhat Begum|HTML_CSS|Bikes-for-refugees| Week 1 #477

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BakhatBegum
Copy link

@BakhatBegum BakhatBegum commented Sep 23, 2023

I replaced the div's with semantic HTML tags.
I replace broken image links.
I added a style button.
I added Flexbox and hovered over the article class.

@netlify
Copy link

netlify bot commented Sep 23, 2023

Deploy Preview for cyf-bfr ready!

Name Link
🔨 Latest commit f3a3709
🔍 Latest deploy log https://app.netlify.com/sites/cyf-bfr/deploys/65155f1799fedb00089536d2
😎 Deploy Preview https://deploy-preview-477--cyf-bfr.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@BakhatBegum BakhatBegum changed the title NW6 - HTML_CSS- Bakhat Begum- Week 1 NW6 - HTML_CSS- |Bakhat Begum|HTML_CSS Week 1 Sep 25, 2023
@BakhatBegum BakhatBegum changed the title NW6 - HTML_CSS- |Bakhat Begum|HTML_CSS Week 1 NW6|Bakhat Begum|HTML_CSS|Bikes-for-refugees| Week 1 Sep 25, 2023
Copy link

@eliza-an eliza-an left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good attempt, but for next time try to pay more attention to the details! When you are a developer, that's your main job, to recreate the image they gave you exactly! You want to notice all of the small details. A good first attempt overall! You did well with the semantic HTML, adding the images and using the hover effect, but the rest of the criteria needs a little more work! :)

Providing donated bikes and accessories to refugees and asylum
seekers in Scotland.
<figcaption class="article__content">
<h3 class="article__title">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have a look at the image provided to you. Your title looks a bit squashed to the top. How would you fix that? Maybe with some padding?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Eliza, I committed all those mistakes that you mentioned.

index.html Outdated
class="header__logo"
class="article__thumbnail"
src="images/spring-fundraisers_thumbnail.jpg"
alt=""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no alt tag here!
Also, have a look at your page and the page given. Does yours look the same? Try playing around with the padding of this element in the CSS!

index.html Outdated
<img
class="article__thumbnail"
src="images/bikes-for-refugees_logo.jpg"
alt="bikes image"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to say "image" in the alt tag. The alt tag is made specifically to describe images, so adding the word image is unnecessary. It's important to keep the information as concise but descriptive as possible here!

index.html Outdated
<img
class="article__thumbnail"
src="images/edinburgh-damascus_thumbnail.png"
alt="image for Help us cycle 4,797km"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same "image" comment as up there ^

background-color: #c05326;
color: white;
border-radius: 5px;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in here, add " border: none " here to remove the black outline on your buttons. Do you see how yours has one and the sample image doesnt?

styles/style.css Outdated
}

.article__title {
margin-bottom: 0.5rem;
font-size: 1rem;
font-weight: 700;
font-weight: 600;
padding-left: 20px;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a look at the sample image, do you see how their lines align at the sides? You dont really want your title to stick out the way yours does!
Their image:
image

Yours:
image

How can you fix this? Have a look at your padding-left property here!

styles/style.css Outdated
@@ -108,7 +132,9 @@ p {
/* Alert */

.alert {
border-style: solid;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sample image does not have a border here so that's not needed :)

styles/style.css Outdated
@@ -59,15 +64,19 @@ p {
}

/* Navigation */
nav {
padding-right: 30rem;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see how your navbar skews to the left? That's because of this padding-right there! Try removing it and see if it looks more like the image

styles/style.css Outdated

.btn {
background-color: white;
color: orange;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of the page looking more synchronous, you always want to use similar colors. This orange doesn't match the one we used for the other button! Play around with the colors and the font-weight of this!!

background-color: var(--grey-light);
padding-left: 30px;
}

.hero h1 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Play around with the styling: padding and margins here. This doesnt look exactly like the picture!

@BakhatBegum
Copy link
Author

I made changes in all the syntax that you have mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants