Skip to content
OverviewSolutions
0
Comments
4
P
Kamil
@Owczarek-Kamil

All comments

  • Kamil•40
    @Owczarek-KamilPosted about 1 year ago

    Hello,

    In my opinion, there is a huge issue with accessibility for the keyboard-only users. The only focusable element is the "Explore" button on the homepage. Besides that, I was not able to navigate throughout the page with my keyboard :/

    0
  • Kamil•40
    @Owczarek-KamilPosted over 1 year ago

    Hello @abhinav

    To center the content in desktop mode:

    1. Remove min-height: 140vh from the body
    2. Remove height: 100vh; from the .container

    and this should do the trick

    In case of moving cards when hovering the buttons. I see that when button is hovered you apply the following styles: 'border: 1px solid white', this 1px of border makes the card move.

    To solve it just apply 'border: 1px solid white' as a regular style to the button (not only when button is hovered or focused on)

    Marked as helpful
    1
  • Mark•180
    @Iron-MarkSubmitted over 1 year ago
    • I would be happy to receive comments, criticism, and such that could improve the website:
      • Cleaner Code
      • Better Practice/Approach to making this website.

    I struggled a bit with how to make containers responsive (efficiently ) that I could reduce the adjustment and use of media queries.

    3-Column Preview Card Component (Mobile-First Approach + Flexbox)

    2
    Kamil•40
    @Owczarek-KamilPosted over 1 year ago

    Hello,

    I am a newbie myself, so do not take my words for granted, but you should not nest buttons and <a> tags together (this also have been pointed out in the FrondEndMentor Accessibility report and HTML validation report). In this case, in my opinion you should go with <a> tags and style them with pseudo classes like, :link, :active, :hover, :visited.

    Also, one can argue whether you should put 'alt' on the icons. For me, those icons do not represent any UX purpose - their purpose is decorative one. In such case, alt should be left blank, and you should include aria-hidden="true".

    And last but no least, next time try to implement some hover animations on buttons/links. Right now the transition is instant and it is not very pleasing for the eyes :)

    Simple transition: all 300ms; would do the work.

    Hope you find my comments useful and like I said, do not take my words for granted :)

    Happy Coding!

    Marked as helpful
    1
  • Kamil•40
    @Owczarek-KamilPosted almost 2 years ago

    Hey @ahmedsaliuGit

    For centering 'per month' I would use quick flexbox as following:

    .subscription-price { display: flex; align-items: center; } and to create gap between '$29' and 'per month' I would just use margin-left.

    I am a beginner myself, so I am not sure if it is the best solution :)

    0