-
Notifications
You must be signed in to change notification settings - Fork 0
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
Complete project code #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Nasirkhan294 🙋♂️
Good job working on your Global summit. You have done the design to the best expectation and incredibly well. 👏
You still need to implement few on the project but you are almost there!!! 🚶:
TO HIGHLIGHT:
- Your project is professional ✅
- Professional README file 💯
- Your PR is professional ✅
- Well-structured files ✅
STATUS: Required Changes ♻️
Required Changes ♻️
- 🟢 Please record the presentation of the application as the project requirements suggest including the piece of code you are proud of.
- Kindly consider the inline review comments.
Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread or slack channel if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
HAPPY CODING AND GOOD LUCK 🤞 🍀
<div class="container"> | ||
<div class="hero-content"> | ||
<p class="hero-top-desc">"Hello, Sharing World"</p> | ||
<h1 class="hero-heading">Converge and Create Global Summit 2023</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You have done a great job using the
h1
tag for the heading. But remember we have to follow the provided design guidelines for this project. Therefore I suggest you ensure to put the image inside the heading text. This will make your application an attractive user interface☺️ . You should also consider increasing the size of the heading to match the design.
<p class="hero-highlight"> | ||
Join a global movement for positive change in Pakistan, uniting | ||
80+ countries in joyful celebration of openness and sharing. | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- As you can see in the original design, I think it would be a great idea to adjust the width of the description such that its width is smaller than the heading to match the original design
☺️ . - In addition, kindly try as much as possible to use the colors that are provided and apply them as seen in the design. For example the background and text colors should be changed.
index.html
Outdated
<div class="feature-container flex-column align-center"> | ||
<div class="feature-cards grid column"> | ||
|
||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing the see more
button was a very great achievement. It shows a better understanding of javascript. Again, at this point, we are looking the design of the application. I have noticed that in the mobile version
of your application, the speaker's cards overlapping each other. This affects the UI of your application, and therefore i suggest that you consider applying the spacing to ensure that the cards don't overlapp.
css/main.css
Outdated
.footer { | ||
padding-top: 3em; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- As the project requirements suggest the footer should contain a logo aligned to the left and text aligned to the right. This is to ensure we have the best UI and also match the original design of the application.
- In addition, the footer in the about section of your application should have a dark background instead. Kindly apply this to match the design 😎.
Your design
Hello @Nasirkhan294 - [ ] 🟢 Please record the presentation of the application as the project requirements suggest including the piece of code you are proud of and share the link in the readme file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Nasirkhan294
In my opinion, your project is ready for presentation! There is nothing else to say other than... it's time to merge it
Good luck! 🎉
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
All the linters setup
Modified HTML code with Home and About pages
Stylized both pages with CSS
Updated Readme File
Implemented all the changes required by the reviewer
Provided link for project intro in readme file under project demo link.