Steven Velazquez - Bootcamp Solutions#105
Steven Velazquez - Bootcamp Solutions#105StevenVelazquez wants to merge 5 commits intosimpleviewinc:masterfrom
Conversation
Week 1.1 - HTML Week 1.2 - CSS
|
I just realized that the domain for the placeholder images doesn't work anymore. I also updated the documentation on the simpleviewinc/cms-bootcamp/ to use the updated asset urls so you're welcome to rebase your fork to pull the updated information as well. Otherwise, use these instead. Week 1.1 - 2. Create a Responsive Image - ResolutionImage Resources
The default image resource is: https://placehold.co/400x180 Week 1.1 - 3. Create a Responsive Image - Aspect RatioImage Resources
The default image resource is: https://placehold.co/540x800 |
Update the IMG URLs Week 1.1 - 2 Week 1.1 - 3
Week 1.3 - JavaScript Exercise 1,2,3 and 4
|
Images has been updated with the new URLs |
JS 5 & JS Data Fetching Exercise CSS Grid & Flexbox Exercise
There was a problem hiding this comment.
I did realize that the originally provided placeholder assets weren't working.
It looks like you've still got some syntax issues on this one. Can you correct that?
Also, since the provided asset urls are broken, lets update the asset urls to these:
| Size | Resource |
|---|---|
| XL | https://placehold.co/2000x900 |
| LG | https://placehold.co/1600x720 |
| M | https://placehold.co/1200x540 |
| S | https://placehold.co/800x360 |
The default image resource is: https://placehold.co/400x180
There was a problem hiding this comment.
Yes, they are in the second commit
There was a problem hiding this comment.
There are also some similar syntax issues on this one. Can I have you fix that?
Lets update the asset urls for this one too:
| Size | Resource |
|---|---|
| LG | https://placehold.co/1600x720 |
| M | https://placehold.co/1200x900 |
The default image resource is: https://placehold.co/540x800
There was a problem hiding this comment.
Yes, they are in the second commit
| <h2>Phase 4:</h2> | ||
| <h1>Distrination Thrive</h1> | ||
| <h3>Objective: Create omni-channel synergy</h3> |
There was a problem hiding this comment.
For SEO and assistive technologies, the heading structure here is incorrect.
Here's a source that can further explain:
https://frontdose.com/posts/html-tag-order/
There was a problem hiding this comment.
I updated the structure with the correct order
| <section> | ||
| <h4>Time</h4> | ||
| <img src="clock.png" alt="clock icon"> | ||
| <p>approx. 24 months and beyond</p> | ||
| </section> |
There was a problem hiding this comment.
So, with the comp showing the clock icon inline with the "Time" heading, there are probably more optimal ways for this html to be structured. Some example options: Wrapping the h4 and img elements in a parent element to group them together or simply moving the icon into the h4 element.
If you think about how it would be styled later down the road, it is possible to achieve the comp's style with the structure provided, but I would argue that there are "better practices"
Side Note:
This comment is more of a nitpick, rather than saying you did it wrong. However, this would be the same kind of feedback I'd be providing you if this was work on an actual widget for a client.
There was a problem hiding this comment.
I submitted a commit with the changes, can you verify if it's correct?
Mock Up a Design comments
2 participants
Week 1.1 - HTML
Week 1.2 - CSS