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

Feedback #1

Open
wants to merge 19 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 19 commits into from

Conversation

github-classroom[bot]
Copy link
Contributor

@github-classroom github-classroom bot commented Jan 8, 2024

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to main since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

  • Click the Files changed tab to see all of the changes pushed to main since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
  • Click the Commits tab to see the commits pushed to main. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @ikaur02

github-classroom bot and others added 10 commits January 8, 2024 02:00
first commit
merging code from feature branch to main branch
Made changes to the icon colors, remove extra property added to p, added two more sections to services page and added links respective to each section
Stylesheet, services page and updated icons
six image manipulation
Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

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

These are some detailed comments on the code. We will put a paragraph summarizing the feedback and the mark in Turnitin separately.

Copy link
Member

Choose a reason for hiding this comment

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

The homepage file should generally be named index.html so that it is automatically picked up by browsers looking at the URL for the website. If you go here, it doesn't exist, but usually this link should work if you have an index.html file.

https://birkbeck2.github.io/web-development-website-project-ikaur02/

You have to go here instead, which is not intuitive:

https://birkbeck2.github.io/web-development-website-project-ikaur02/portfolio_website.html

<li><a href="portfolio_website_projects.html">Projects</a></li>
<li><a href="portfolio_website_contact.html">Contact</a></li>
</ul>
</nav>
Copy link
Member

Choose a reason for hiding this comment

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

Pretty good

<div class="flex-container_footer_second_div">
Reach me out at [email protected]</div>
</div>
</footer>
Copy link
Member

Choose a reason for hiding this comment

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

These are called "flex container" but there are no flex display rules applied. Generally the CSS names should be easy to read to get a sense of what is happening in the code, but this does not give the right impression.

Decent basic use of semantic HTML for the footer.

<div class="work_01_first_div">
<p>I do experiments & write it down called as Research projects!</p></div>
<div class="work_01_second_div"><img src="images/KFC_cover.png" alt="KFC project cover image"></div>
<div class="work_01_second_div"><img src="images/Upaj_website_cover.png" alt="Upaj website cover image"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Good for using alt text

</div>

<div class="flex_container_work_02">
<button>View more</button></div>
Copy link
Member

Choose a reason for hiding this comment

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

Good that this is a semantic button element, but it doesn't do anything. What should happen when the user clicks this? The interaction design is not clear to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The user will be directed to projects page and able to view all the projects

{
display: flex;
height: 150px;
flex-direction: column;
Copy link
Member

Choose a reason for hiding this comment

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

Good use of media queries to change the flex direction!

height: 3rem;
border-radius: 30px;
margin-top: 20px;
box-shadow: 0px 5px 15px 0px rba(28,0,181,0.3);
Copy link
Member

Choose a reason for hiding this comment

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

Where did this code come from? It is not taking effect in the website because rga is not a valid CSS function.

But the point is that this should be explained or a source cited.

{
box-shadow: 0 0 2px 1px rgba(0, 140, 186, 0.5);
filter:grayscale(100%)
}
Copy link
Member

Choose a reason for hiding this comment

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

These classes are named with 01, 02, 03, and very similar names like "flex container" at the beginning of every one. This makes it very hard to read the HTML and compare it to the CSS, and so it makes it harder for you to debug when there are issues. Next time try writing more expressive names, like gallery and gallery-item. If you want to differentiate the small and big galleries, you could have big-gallery, big-gallery-item, small-gallery, small-gallery-item.

filter:grayscale(100%)
}

/*Media Query for Mobile - Global styling*/
Copy link
Member

Choose a reason for hiding this comment

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

You have another media query for global styling of mobile above. Is it duplicated?

max-width: 40%;
}

/*Media Query for Mobile - Global styling*/
Copy link
Member

Choose a reason for hiding this comment

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

Again the media query for global mobile styling is duplicated.

ikaur02 and others added 2 commits April 20, 2024 21:05
This reverts commit 40032bb, reversing
changes made to d8c98b5.
Revert "Merge pull request #3 from Birkbeck2/Full_code"
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