David Lemvigh
@dlemvighAll comments
- @MichalTrubiniP@dlemvigh
The visual
- There is a scrollbar near the faces
- The purple buttons are a bit brighter in color that design
Generally really good solution, nice and responsive on all screen sizes.
The code
- Nice semantic html
- Well organized css classes
- An elegant solution to the desktop faces. The design shows the faces closer to the edges. But with your solution that could easily be changed by adding
justify-content: space-between
to the.banner__container
. (I also like your design better than the figma design on that front) - I think the cause of the scrollbar is the
overflow-x: hidden
, which I don't understand the reason for. Removing it removes the scrollbar. (It is not the first time I've see overflow-x/overflow-y having unexpected interactions, but I don't know/remember why)
Note: I tested in Chrome on Windows 11, if that helps with debugging the scrollbar.
- P@okayishmaelWhat are you most proud of, and what would you do differently next time?
Mixing grid and flexbox to achieve my goal.
What challenges did you encounter, and how did you overcome them?The design indicated 13px font size.16px/ 1rem is what I used for readability. I some people can see to well. I am just advocating for users
What specific areas of your project would you like help with?My is seems a little bigger.
P@dlemvighThe Visuals
- Looks really good
- Avatar outlines a bit different than design, but looks good
- Nice 4, 3, and 1 column layouts
- A bit extra body margin on mobile layout compared to design, making the content very squezed on when get below 400px
- Missing "watermark" on purple card (also found it annoying to add myself without getting it on top of the text, but find a nice way by setting
background-image
on the card).
Visually really good, only small notes
The Code
- The is an attempt at css grid, but then a lot of flexbox inside doing most of the work (
grid-template-columns: repeat(6);
- repeat is missing its second parameter so gets ignored) - A bit of grid and flexbox nesting, would have been nice to have been done with a single grid. Maybe using
order
to put the "kira" card on the bottom. - Well structured html
- Nice use of css variables
Flexbox usage really good. Grid usage could have been better. But in general really nice solution
- P@CooleyWCWhat are you most proud of, and what would you do differently next time?
This was my first time using Sass for CSS
What specific areas of your project would you like help with?I know my box-shadow for the cards isn't quite like the Figma design - I could use help with this.
P@dlemvighThe visual
Looks great, minor discrepancies from the design, but it feels more like a stylistic choises than mistakes.
Nice use of CSS grid
The code
Nice clean HTML, the only petty comment is that you have multiple h1 headings which might not be the best for accessibility, but I could be wrong on that.
The scss files are generally well structured. There is a bit of .hero styling that has snuck into _typography.scss which is confusion, and probably should be in _hero.scss.
Marked as helpful - @nada123432hP@dlemvigh
** The visuals **
- many visual differences from the design ** background color ** fonts ** image size
- the layout kinda work on exactly the breakpoints 320px, but looks pretty broken on most other width < 460px
** The code **
- A lot of inline-styles
- Mix of classes and inline-styles
- Does use flexbox and media queries for layout
- @Levi271997P@dlemvigh
Visual
The good
- Looks great, really well done.
- Love that you have added a loading state that wasn't in the design.
- Audio playback worked really well
The bad
- font switcher is a bit off, options are light in dark-mode and dark in light-mode
- keyboard accessibility are missing a few things ** dark mode can be toggled, but there is no focus outline ** font switcher cannot be accessed using keyboard navigation (tabbing throught the page). *** could potentially be fixed by adding tab-index=0, focus styling, and keydown event handler
Code
- 400 lines of react in one file is a lot, would be better if it was split up into at least a couple of separate files
- looks like it is a create-react-app which has been deprecated and not updated since 2022
CSS
The good
- I like the of variables and the mixings
- especially the $typography-styles, letting you mimic the figma usage of defined typographies
The bad
- 700 lines of css in a single files is a lot.
- a lot of nesting in the rules. This causes increased specificity which can make extending the classes in the future difficult, without
!important
and other hacks. - I'll be honest I got overwhealmed by everything being in one big filed, and haven't reviewed it that well.
- @davistar21P@dlemvigh
the good
- I am impressed at that the page is implemented in vanilla js
- well organized html (minor mismatch in label for mixing kebab-case and camelCase)
the bad
- focus styling a bit off for year and interest rate, looks different than the focus styling for amount
- error styling a bit off. The input addon ($ , year , %) does not have red background.
- radio inputs have no focus styling, so keyboard users have no outline to show focused element.
- no validation messages are shown ("this field is required")
- clear doesn't clear validation state. When clicking clear all, I would have expected the validate state to also be cleared.
- "clear all" not keyboard accessible (could be fixed with tabIndex="0") or making it a button with styling
conclusion
A solution that fulfills the technical requirements, with semantic HTML. There are many deviations from the design in terms of styling.
Marked as helpful - @khlv2219What are you most proud of, and what would you do differently next time?
I was very proud of recreating this design without any further help of external resources.
What challenges did you encounter, and how did you overcome them?My biggest challenge was dealing with position/display properties of the table. I tried very long different properties and finally got the solution I wanted.
P@dlemvighPaise: You really nailed to desktop design (way more than I could be bothered myself).
Comment: The mobile design seem to be missing, where the margin and border-radius is removed (among other things).
Comment: Your use of css variables is pretty good. Except the font-weights
--fw-1-400: 400;
with this naming convension it ends up being a longer way of writing 400, and if you wanted to change the weights in the future you would end up changing the names to match. - @IfeoluranmiWhat are you most proud of, and what would you do differently next time?
It was nice working on this challenge as i attempt the Idea Test being suggested by Frontend which is making visitors to navigate links via keyboard only, it's quite fun trying out the challenges as it put my knowledge to test, learning and relearning stuffs that i might have forgotten.
P@dlemvighThe HTML is nice and accessible, but the design have few missing a few things. The links does have a focus state, but not the one from the design. In your case you could have used
li:focus-within { }
to style the list items if the links inside have focus.There is no hover state on the links.
There are also some side padding on the card missing.
Nice touch with the ease-in-out transitions
- @MacNkhataWhat are you most proud of, and what would you do differently next time?
I am proud that I reduced the use of divs as pointed out in my last submission. This has helped to make my html files concise and easy to read.
What challenges did you encounter, and how did you overcome them?I had troubles using the static fonts provided.
What specific areas of your project would you like help with?I dont have for now
P@dlemvighVery clean solution. No notes
- @PattykevWhat are you most proud of, and what would you do differently next time? What I learned
- How to auto values om margin properties :
In CSS, the
margin
property is used to create space around elements. When you set themargin
toauto
, it allows the browser to automatically calculate the margin space. This is particularly useful for centering block-level elements within their container.
Here's what
margin: auto;
typically means:-
Horizontal Centering: When applied to a block element (like a
div
), setting the left and right margins toauto
allows the element to occupy the available horizontal space equally on both sides, effectively centering it within its parent element..centered { width: 50%; /* or any other width */ margin: 0 auto; /* top/bottom margin 0, left/right margin auto */ }
-
Vertical Margins: The
margin: auto;
setting does not have the same effect for vertical margins (top and bottom) unless certain conditions are met (like using flexbox or grid layouts). -
Flexbox and Grid: In flexbox or grid layouts,
margin: auto;
can also be used to distribute space between items or to push items to the edges of their container.
In summary, using
margin: auto;
helps in controlling the layout and positioning of elements within a webpage, particularly for centering elements horizontally.- Remembered how to change local repository url
git remote set-url origin <put the new url there>
- How to insert an image with markdown
What specific areas of your project would you like help with?
In CSS learning.
P@dlemvighGenerally a nice and clean approach.
On the
.container
you setmargin: 9rem auto;
. As you correctly stated the sets theauto
used for both left and right margin centering the content. The css shorthand you use also use9rem
for both top and bottom margin, where the bottom margin might not be intentional (on mobile it could add a scrollbar due to the short height of the page).For the container you also define a fixed height for the container itself
height: 32rem;
. I would suggest just removing the height, and let the content inside determine the height of the container. That would make your solution more robust to future changes. In your case you would have to add some bottom margin to the last element to get the proper padding.The last point would be to maybe set padding on the
.container
instead of having left/right margin on all its child elements.Marked as helpful - How to auto values om margin properties :
In CSS, the