
Caleb Bankole
@BankoleCalebAll comments
- @wonuola-y@BankoleCaleb
Hi. You did a good job on this project and congratulation on your new job - I saw it on LinkedIn.
First, I realized that not all part of your "go back to notifications" button is clickable. User has to click on the text itself. You can look it up Also, you are logging a message to the console. You should have cleared all your console.log() before deploying.
Thirdly, when you click the "go back to notifications" button, your home page still shows three notifications unread even after one has read it before clicking a notification.
Good job and keep coding. How was your UTME score? Did you attend Adams' College post-UTME class?
Marked as helpful - @Nduoyoosung@BankoleCaleb
Keep up the hard work! It is a very beautiful and responsive site.
Anyways, you could improve the functionality of the pink transitioning button when hovered. Instead of changing the top margin, you could have just transitioned it and it does not have to be up to 25px - it is making it flicker when the mouse it at some point around the button (you can confirm that yourself).
By the way you really did a good job; keep up the journey!
- @Silas310@BankoleCaleb
Well done, you have done good job but I believe it can be only be better. There are a lot of things to make better here, but I'm just going to share a few things.
First, you didn't have to use three different CSS files; you could have just adjusted styles using media query. Also about your script, its is no longer in vogue to use var declaration. Furthermore, instead of having a long chain of "if else" statements, you could have just used a switch block. By the way, most of the things you used js for could have been easily done with CSS.
I hope you didn't find this review criticizing. Keep working and I can't wait to meet you at the top.
Marked as helpful - @Coolbylaki@BankoleCaleb
Good job! No doubt, it is a really easy project, but you must always put every little details into account - like centering the paragraph and "star icon", which states otherwise according to the design.
Marked as helpful - @BankoleCaleb@BankoleCaleb
@Kamlesh0007 please, help me take a look at the code of this project and how I can improve it.
- @Olarte910903@BankoleCaleb
Well done on your first project!🎉🎉🥳
First, I'd like to tell you that you don't need flexbox on the div. You only needed it on the body for centering. Also, using semantic HTML could have improved the accessibility of your site. Furthermore, the page is not responsive.
You didn't have to use more than one CSS file as you have done; you could have just used media query. With what you have done, no style is applied on small screen - confirm that.
I hope you find this helpful. Continue on journey; we'll meet at the top!
Marked as helpful - @Derajatul@BankoleCaleb
You did it! It is awesome, you are awesome!
Anyways, there is always room for improvement... First, you did not follow the design for the desktop view. Also, you did not spell 'careers' correctly (although that is minor😊) You should also be conscious of the style on the "learn more" button - the hover state.
Again, I must commend you for the wonderful work done. We'll meet at the top!
Marked as helpful - @Kamlesh0007@BankoleCaleb
Nice work!
Although it could have been more efficient if you something like CSS variables for the different color mode instead of changing the style file.
Also, there is a repetition of a code above on line 107. Is there really a need for that?
Marked as helpful - @Ayotommy012@BankoleCaleb
Of course, it needs adjustment. Especially the centering.
Since you have decided to use flexbox on the body like that, you can also change the flex direction so that the two block would stack on top of each other, rather than side by side as it is now.
Also, you did not seem to use the right colors. The text is not readable.
You have done a very good job, kudos. Just a little more touch.
Marked as helpful - @jamesaking02@BankoleCaleb
For positioning the background images at the top left and bottom right, you'd use this style:
background-position: 0% 0%;
That's basically the format, I think you should be able to edit the value to suit your need. You could also use negative values. Note that you are putting the style on the body.
background-position: left top;
- @wonuola-y@BankoleCaleb
Hello again, Wonuola. First, I must commend you for taking up the challenge (not everyone has the mind to).
Anyways, there are so many areas to work on. If you don't mind, we can do it together.
To start with, the images are too big that there is scrolling even on large screen. Also, the links are not well styled as they don't seen clickable. I know it is just a demo site, but you can still add basic styles like "cursor: pointe;"
There are other observations, but you can chat me on WhatsApp if you like to link up.
08134690893
- @wonuola-y@BankoleCaleb
Good job, Alonge.
Anyways, you can improve the experience by changing the cursor types of the logos, register, login button.
Also, the image is too large for mobile view. A trick you can use is to change the "width" which you set to 100% to "max-width". I promise, it would be a magic!
- @wonuola-y@BankoleCaleb
Alonge, you have done a very good job. I guess you were a starter when you did this. I admire your learning journey.
Of my many observations, the one I would like to pass is the contrast for the text color and background color of the "why us?" section. Don't you think it would be nice if you use another color, or yet follow the design.
Once again, good job!
Marked as helpful