diff --git a/comments.MD b/comments.MD new file mode 100644 index 0000000..d7bdfe3 --- /dev/null +++ b/comments.MD @@ -0,0 +1,14 @@ +# Comments / things that stand out + +- Grid is always 12 columns +- For the first task, I just changed classNames, because its the easiest solution. +- For the second task, I moved the markup of the icons down, also removed some CSS that added element-spacing to the bottom of the item. I realize it's already handled by a `:last-child` selector in the _critical.css but I think for maintainability and readability sake it's better to also remove it from the component specific styling. I think if a new element was introduced underneath, it would need to be re-evaluated anyways with regards to how much whitespace there should be between items. I also made it so that the icons are aligned at the bottom with each other to make the visuals seem not as chaotic. +- Why is 1 of the USP items a link while the others arent? for now I'll just assume it was a mistake and remove the link element. The only reason I'm doing so is because I want to add flex and I would have to add flex to 2 different items. +- For the reviews section with regards to headings; I think there might need to be some change anyway; none of the texts on their own are a very good indicator of the section in the page structure. Similarly, the USP's could do with a (visually hidden) heading 2, and potentially the actual USP items could get a heading 3 (their title) +- Also no completely sure about the headings in the slider hero. +- For the touch target size, I've upped the size on mobile. The button is still touching the edge of the input field.I don't know if this is intentional design wise, so I left it (I checked the initial styling). Also, the + is falling out of the button, so I added some markup + styling to fix this. +- While making the new section I tried to use as much existing styles as possible, and for the design implementation I had to eyeball some values, one of the things I eyeballed is the margin between image and content on mobile. I did not do it the same way as it's done in the `two-column-info-block` slice, which uses margin as a %. Instead I used a static value, as I am not sure what your guys' philosophy is on this. +- For the responsiveness of this section, I tried to use the same way of thinking as was used in the `two-column-info-block`. To be honest the sections are now very similar but I would imagine ina. real life situation there wouldbe some back and forth regarding design. +- For the collapsable footer, I have some improvements /comments if I had a little more time. Firstly, an animation on the closing/opening would be nice. Second, I have the feeling I might have to dive a little deeper in the markup and stying as I saw some `collapsible` class already pop up somewhere. +- I implemented the scroll functionality with JS, though we could also implement an anchor to scroll to and add the scrolling-behaviour to the body. +- For making sure that content is always readable, I added a padding-top with currently a set in stone value, which is not based on any vars. I think in some cases it's worth considering making a calculation out of this (for example, padding-top and bottom of nav item + font-size). \ No newline at end of file diff --git a/src/index.html b/src/index.html index 1b85c24..b71937b 100644 --- a/src/index.html +++ b/src/index.html @@ -338,7 +338,7 @@