-
Notifications
You must be signed in to change notification settings - Fork 30
Estefania Jacobo #26
base: master
Are you sure you want to change the base?
Estefania Jacobo #26
Conversation
import "../styles/clouds-section.scss"; | ||
import "../styles/newsletter-section.scss"; | ||
import "../styles/footer.scss"; | ||
import "../styles/responsive.scss"; |
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.
Nice way of separating the css rules
|
||
function modifyBlogData(currentBlog) { | ||
var textSection = document.getElementsByClassName('blog-section__content-text'); | ||
var titleText = textSection[0].getElementsByTagName('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.
Remember that selecting HTML nodes directly is not a good practice, using classes is preferred in case the site structure changes.
x[i].style.display = "block"; | ||
} | ||
var image = document.getElementsByClassName("blog-section__content-image"); | ||
image[0].src = blogData.articles[currentBlog].images.desktop; |
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.
When you manage responsive images in this way you'll have problems when users don't have js turned on, I'd recommend to use HTML native responsive images instead https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images
} | ||
function displayCustomer(n) { | ||
var i; | ||
var x = document.getElementsByClassName("customers-section__images"); |
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.
Just as a recommendation, when selecting nodes with js is better to do it with selectors that are not related with styles and use a prefix, ie: .js-my-class-selector
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.
I made some comments and also noticed that some sections responsiveness break in some specific screen sizes, but in general the project complies with the basic requirements.
What's your progress on the project?
Where should the reviewer start?
The responsive part was worked until the blog section.
Missing styling and most of the responsive design for tablet
Checklist
console.logs