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 | nazanin-saedi| BikeForRefugees | [TECH ED] Bikes for Refugees | week1 #483

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

Conversation

nazaninsaedi
Copy link

No description provided.

@netlify
Copy link

netlify bot commented Oct 7, 2023

Deploy Preview for cyf-bfr ready!

Name Link
🔨 Latest commit 346a768
🔍 Latest deploy log https://app.netlify.com/sites/cyf-bfr/deploys/652138f0657c830007d55b66
😎 Deploy Preview https://deploy-preview-483--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.

Copy link

@Ara225 Ara225 left a comment

Choose a reason for hiding this comment

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

Hello!
Congrats on your first pull request! Seems liek you've done most the work, you just need to move style.css into the styles folder and you'll be almost there :) Thanks

@@ -64,14 +64,13 @@ <h1>Bikes for Refugees</h1>
Providing donated bikes and accessories to refugees and asylum
seekers in Scotland.
</p>
<button>Donate a bike today</button>
<button>Volunteer</button>
<button class="donated">Donate a bike today</button>
Copy link

Choose a reason for hiding this comment

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

Descriptive class names :) You need to style these a bit diffrently though to look like the image but I am sure you are aware of that. Were also need background images

<img
class="article__thumbnail"
src="spring-fundraisers_thumbnail.jpg"
src="/images/spring-fundraisers_thumbnail.jpg"
Copy link

Choose a reason for hiding this comment

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

Good fix


:root {
--grey-dark: #292b2c;
--grey-light: #e4e4e4;
Copy link

Choose a reason for hiding this comment

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

Good that you're using CSS vars :)

/* hero */

.hero {
background-image: url("../images/header-bike.jpg");
Copy link

Choose a reason for hiding this comment

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

I'm afraid this link is broken, it will work in the other CSS file

/* Alert */

.alert {
background-color: var(--orange-light);
Copy link

Choose a reason for hiding this comment

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

All the styles here look really good

@@ -0,0 +1,295 @@
/**
*
Copy link

Choose a reason for hiding this comment

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

This is the wrong CSS file I'm afraid - the one in syles is the one you want this is why the styles aren't showing up. I think the work you have done is very good though, it's very close
https://github.com/CodeYourFuture/bikes-for-refugees/blob/main/styles/style.css

Copy link

@MaryamAlattal MaryamAlattal left a comment

Choose a reason for hiding this comment

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

Good work 👍 suggestions:
Navbar should be properly styled (Position/spacing/Color text/hover effects)

Copy link

@MaryamAlattal MaryamAlattal left a comment

Choose a reason for hiding this comment

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

Well done to use background-image in class hero👍 Recommendations:

Check the correct path (../images/etc); With the same class hero, the style of the buttons needs to be changed

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.

None yet

3 participants