Github-tracker Nina Rivera#54
Conversation
waliem
left a comment
waliem
left a comment
There was a problem hiding this comment.
Good job Nina on this project! You made it work and you made a very cute site! Everything works when I test the site and you fulfill the requirements so yay, be proud!
Only thing I saw was that I think that there is some styling that conflict each other or doesn't actually do anything so a tips is to just inspect the page and untick the orange boxes next to the elements and styling to see what actually fills a function for the styling. but its nothing that disturbs the page! it would just clean up the code a bit when you remove some code that just takes space. otherwise you have a very clean and nice code to read, it was easy to follow, good job! 😃
| </div> | ||
|
|
||
| <main class="Projects"> | ||
| <h2 id="projects"></h2> |
There was a problem hiding this comment.
it doesn't seem like this h2 is visible in the page? is it maybe behind some object or just invisible? 🤔
| }) | ||
| } | ||
|
|
||
| function sortingFunctionFromStackOverflow(a, b) { |
There was a problem hiding this comment.
This is a very nice function! I forgot to use it, I really should add it. 😊
| border-radius: 50%; | ||
| width: 200px; | ||
| margin: 15px; | ||
| box-shadow: -30px 30px 30px rgba(0, 0, 0, 0.3); |
There was a problem hiding this comment.
It's nice that you added a shadow to your profile image! the shadow looks a little bit "off" from the shape of the image, a suggestion is to play around a little with the value-numbers of the shadows. I also worked a while to figure out the numbers to make the shadow, its tricky! 😊
| } | ||
|
|
||
| .projects { | ||
| flex-direction: column; |
There was a problem hiding this comment.
I like your responsiveness of the project-boxes! Since it seems like you have already declared the .projectbox to be display: flex I think that the display: block doesn't do anything in this part? I tried to remove it in the inspector and it seems to no change anything. just a suggestion of you want to have a little bit less code 😊
| .chart-class { | ||
| width: 50%; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; |
There was a problem hiding this comment.
The chart was very tricky to style because it doesn't really listen to whatever you style it to like other element, I also tried to style it with flex and with different align-item and justify-content etc but nothing actually happened. The only thing that seems to work is the margin: auto; that you have, the other properties could actually be removed I think. (could be worth try to remove it to have a cleaner code) I also just set a width in % and then align-self: center to make it centered and not being huge, that worked for me at least 🙂
| color:#74a09f; | ||
| } | ||
|
|
||
| @media (min-width: 668px){ |
There was a problem hiding this comment.
you have great responsiveness in mobile and tablet but I think it would be great to add another media query for desktops and bigger, it works as it is but the project boxes becomes a little bit too stretched and the chart is huge 😄
| @media (min-width: 668px){ | ||
| main { | ||
| text-align: center; | ||
| flex-direction: row; |
There was a problem hiding this comment.
It seems like this one get overwritten in the inspector tool by the display: block element so it doesn't actually serve anything. tips is to check the inspector and try to remove and add to see if anything happens 😊
2 participants
No description provided.