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 | Rabia Avci | HTML-CSS | Week 1 #472

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

Conversation

RbAvci
Copy link

@RbAvci RbAvci commented Sep 22, 2023

Use semantic markup
Fix the broken images
Style buttons
Add spacing
Hover effects

@netlify
Copy link

netlify bot commented Sep 22, 2023

Deploy Preview for cyf-bfr ready!

Name Link
🔨 Latest commit 9acaa5e
🔍 Latest deploy log https://app.netlify.com/sites/cyf-bfr/deploys/6519fec89b51220008da11ff
😎 Deploy Preview https://deploy-preview-472--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.

@RbAvci RbAvci changed the title North-West 6-Rabia Avci-HTML-CSS-Week 1 NW6 | Rabia Avci | HTML-CSS | Week 1 Sep 22, 2023
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.

Hi,
Some good work here, you're doing very well but also issues I'm afraid. Here's what needs to change:

  • The text and buttons over the big images are not in the same place as the images say
  • All the text in the cards is orange, not just the headers
  • There's far more padding/whitespace between the edge of the page and the content.
    Please try to fix and If this doesn't make sense, let me know

index.html Show resolved Hide resolved
styles/style.css Show resolved Hide resolved
styles/style.css Outdated Show resolved Hide resolved
styles/style.css Outdated Show resolved Hide resolved
@RbAvci
Copy link
Author

RbAvci commented Oct 1, 2023

Thanks for your review @Ara225!

I studied flex layout and then decided to re-work on this task. Therefore I started from scratch and fixed all the issues. I will appreciate it if you can review it again.

Thanks in advance!

@RbAvci RbAvci requested a review from Ara225 October 1, 2023 23:26
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.

Semantic markup was added successfully.

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.

-The broken images were successfully added with the absolute path.
-The required colour in the CSS was corrected.

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.

The buttons were styled properly with a hover effect.

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.

The code was corrected by adding class to style.css, Well done!

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 job by modifying the class.

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.

The transition and hover effects are working properly.

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.

The spacing and fixing positions are working well, but since you used Flexbox try to add a collapsible navbar.

@RbAvci RbAvci closed this Oct 9, 2023
@RbAvci RbAvci reopened this Oct 9, 2023
@RbAvci
Copy link
Author

RbAvci commented Oct 9, 2023

The spacing and fixing positions are working well, but since you used Flexbox try to add a collapsible navbar.

  • Thanks for your review, Maryam. Do you mean to add .navigation__list { flex-wrap: wrap;} ?

Hi, Some good work here, you're doing very well but also issues I'm afraid. Here's what needs to change:

  • The text and buttons over the big images are not in the same place as the images say
  • All the text in the cards is orange, not just the headers
  • There's far more padding/whitespace between the edge of the page and the content.
    Please try to fix and If this doesn't make sense, let me know

-Thanks for your review, Anna. I've made all the changes to the lists above as you've asked.

@Ara225
Copy link

Ara225 commented Oct 14, 2023

Looking really good! I think everything I metioned has been fixed well :)

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.

3 participants