All comments

• 

  Madhavan•340

  @madhavan-tsSubmitted 11 months ago

  I have completed this as my 8th challenge in Frontend Mentors. I am glad to accept any feedback and suggestions regarding the accessibility and the techniques that I have used to develop the webpage.

  I have learnt and used the grid-template-rows and grid-template-columns for making the grid responsive

  .container {
    display: grid;
    grid-template-rows: repeat(2, minmax(1fr, max-content));
    grid-template-columns: repeat(2, 1fr);
  }
  
  @media only screen and (max-width: 650px) {
    .container {
      width: 31rem;
      grid-template-rows: repeat(3, minmax(max-content, 1fr));
      grid-template-columns: 1fr;
    }
  }
  

  Please check this approach whether it is a correct one or not.

  Advance thanks for any comments 🫡.

  

  Responsive Single grid price component using Grid

  2

  50
  

  Miran Legin•680

  @miranleginPosted 11 months ago

  Hi @Madhavan,

  I would remove grid completely from the mobile view, if you disable it yourself in the Dev Tools you will see that it's not doing anything useful.

  Better solution would be to include it in the media query on the resolution you actually need it, in that way you don't need to override anything.

  Also I would like to +1 the previous posters response about width/min-width/max-width. In general it is advisable to not use width/height on elements because that can lead to overflow issues etc...

  Happy coding!

  Cheers, Miran

  1
• 

  Muhammad Desai•640

  @momorocks111Submitted about 1 year ago

  The difficult thing was finding the correct dimensions for the mobile view. It's at 375px when it changes from desktop to mobile, is that normal?

  

  Product Preview Card

  1

  30
  

  Miran Legin•680

  @miranleginPosted about 1 year ago

  Hi momorocks,

  regarding breakpoints sizes i believe they are only recommendations how the page should look like on a given browser width, which doesn't mean if the 375px is the mobile screenshot that everything from 375px higher should be desktop or tablet view. It is up to the developer to judge where is the sweet spot to translate the design from mobile to tablet/desktop and vice versa. Usually when things start to fall apart quite drastically it's time to think of placing appropriate breakpoints in the code.

  For instance I've put breakpoint at the 640px browser width because i found that this is the sweet spot for everything to look good. Keep in mind that there are probably far less devices that have resolutions in a range from 500-600px so not every device width is equally important in that matter. You should focus more on device breakpoint ranges. Here is the quick google for that matter:

  Mobile devices – 320px — 480px. iPads, Tablets – 481px — 768px. Small screens, laptops – 769px — 1024px. Desktops, large screens – 1025px — 1200px. Extra large screens, TV – 1201px, and more.

  Keep coding, Miran

  0
• 

  Kira Weizman Shapira•90

  @kiraweshSubmitted about 2 years ago

  Hello everyone! 👋

  Thanks for checking my solution code to the 3-column preview card component challenge

  At my learning process I learned how to use in nth-child in stylesheet in order to use different styles instead creating more classes.

  .col:nth-child(1) button {
    color: var(--bright-orange);
  }
  
  .col:nth-child(2) button {
    color: var(--dark-cyan);
  }
  
  .col:nth-child(3) button {
    color: var(--very-dark-cyan);
  }
  

  also I learned how to use in outline method for button activity states, because somehow, using in border method, expanded other elements size.

  .col .button:hover {
    background: none;
    outline: 2px solid var(--background-headings-buttons);
    color: var(--background-headings-buttons);
  }
  

  I'm happy to see that after 6 challenges I did, the process of coding became faster for me.

  Built with

  • Semantic HTML5 markup
  • CSS custom properties
  • Flexbox
  • CSS Grid
  • Desktop-first workflow

  

  3-column preview card component

  3

  110
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Kira,

  congratulations on completing this challenge!

  Getting faster and more confident is a nice feeling isn't it?

  Now let's get back to feedback. I won't go into semantics because previous poster reviewed that part. The part i'm most interested is the CSS and i think there are some improvements to be made there.

  1. line 44 of style.css has some invalid properties which can be inspected in Dev Tools, this is on .columns declaration.
  2. you have used border-radius on a single .col elements and make your job more difficult and styles harder to mantain, you can instead put all rounded corners on parent div .columns because no matter what the direction on the cards is they will always have all four corners rounded, also you need to apply overflow:hidden on a parent to stop children's background to spread outside of the corners
  3. most of the spacing here on the platform is based on a 16px value which is 1rem by browsers default, you can apply that logic when dealing with margin/padding spacing stuff, if it's 24 it is 1.5rem/em, 32 is 2rem/em etc. I found the best way to use rem's for applying layout stuff and spacing around text is easier with em because it is calculated based on font size of an element. For example: h1 has a font-size of 24px, and margin bottom of 12px, if your h1 grows on the larger viewport you need to manually enlarge the bottom margin. if you use 1.5rem as a base font size, and put 0.5em as a margin top, when you enlarge you font-size margin will always be 50% of that font size because em is relative to the element's font-size.
  4. i've noticed that you declared your custom properties with this naming convention:

    --lexend-deca: 'Lexend Deca', 'sans-serif';
    --big-shoulders-display: 'Big Shoulders Display', 'sans-serif';
  

  this is somehow limited because if font is about to be changed you would also need to change the name to because it doesn't make any sense to have custom property --lexend-deca which points to open sans font for instance. that way you are playing cat and mouse because now you have to replace all the instances of this font throughout the whole stylesheet and you made yourself a recipe for disaster. Better way would be to name it like a --font-base with --font-heading for example. 5. also i would suggest to take a look at some of the modern CSS resets to just have a clean start each time, you can adjust it to your needs later if you need to or add couple of things if you find necessary

  Andy Bell reset

  Josh Comeau reset

  Both of them did a great job with explaining all the individual rules they created so check them out!

  Keep coding and see you on another challenge!

  Cheers, Miran

  Marked as helpful

  1

  This comment has received 3 repliesShow
• 

  Mohit Mummon•530

  @Mohit-k-MummonSubmitted about 2 years ago

  Hello, I had fun with this project as I learned how to use SwiperJs for creating modern touch sliders. I used a padding container for the two halves of the page the slider intersects because I didn't know how to make the carousel overflow the padding on the body. I appreciate you taking the time to look at my code, any feedback is appreciated!

  

  Design portfolio (SwiperJs)(ParcelJs)(VanillaJs)(VanillaCSS)

  1

  30
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Mohit,

  congratulations on completing this challenge!

  As I've just submitted my take on this one earlier today I have something to compare on to. I think your solutions looks and behaves quite nice. I really like subtle hover effect on Services cards and i would recommend using this kind of effect on loading animations too. My personal feeling is that they are way too much for greeting you when you load the page every time and I would suggest toning it down a bit. Maybe some staggering loading animation on cards would be better idea. Maybe something like this

  I noticed that you have the same issue with vertical alignment of the text in the buttons like i had and the solution to that is quite simple:

  .btn::after {
      content: '';
      display: inline-block;
      height: 1.75em;
      vertical-align: middle;
  }
  

  The problem and the solution are explained on this link

  Keep coding!

  Cheers, Miran

  Marked as helpful

  0

  This comment has received 1 replyShow
• 

  Cheosphere•1,040

  @CheosphereSubmitted about 2 years ago

  ...made with a lot of love 🤘🏻🙂

  

  Coding Bootcamp Testimonials Slider (HTML | CSS | JS Vanilla)

  1

  580
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Cheosphere,

  congratulations on completing this challenge!

  I'm impressed how you did this challenge because in my view there are probably 20% or more code which is definitely not needed here and i think you made yourself life much harder with this one. There are lots of hard-coded values especially on the height of elements and with things like that life can be so much harder when you need to maintain all that stuff.

  What probably bothers me the most is the sizing of the actual buttons for the slider. They are really hard to click because they are to small, you can read some info on setting size on buttons.

  Other that that it would be better to use button element instead of a a tag here also because a have different purpose. It is used primarily for links.

  Also when i click on the slider buttons there is an instant scrollbar to the right side which is causing content to jump from right to left and back an is easily avoidable with probably smaller value for scale.

  Blockquote tag and an H2 could be switched with flex-direction: column is you wanted to switch their places without using negative order on the blockquote itself. Also i'm not an expert it semantics but i don't think that blockquote is used properly here.

  Key takeaway from this is to try and make yourself life easier without fighting the layout so much. You've used all the techniques for positioning so you already have some knowledge but using it rightly takes some time and practice.

  Keep coding!

  Cheers, Miran

  Marked as helpful

  2

  This comment has received 3 repliesShow
• 

  P

  Dave•5,295

  @dwhensonSubmitted about 2 years ago

  This is my second project using React and first using styled components. I put some details in the readme, but some issues/questions I have are:

  • One issue I had with styled components is keeping track of whether the components in the file were now React components or styled components! I also struggled mapping the styled components to the rendered DOM in the dev tools, which made debugging a bit tricky in some cases. How to people manage this?
  • 'React.useEffect` is asking for additional dependencies in a couple of places in board.js (see FIXMEs). The only way I could get the app deployed was to ignore them. If I add them as suggested I end up with infinite loops. I wasn't sure how to deal with this if any one knows how to deal with this I would love to know.
  • Board.js is a bit of a monster, I had a look at refactoring things, but I couldn't see any obvious ways to do this. I considered moving the remaining game logic functions out, but they all change state so this didn't seem sensible. Any suggestions would be welcome!

  Thanks in advance for any suggestions and feedback. Cheers Dave

  

  Tic Tac Toe - React and Styled Components

  2

  80
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Dave, congratulations on completing this challenge!

  I'm no expert in React by any means, even struggle in Javascript a bit so can't comment on that part. So I will comment on some things that I can see and think they can slightly improve user experience.

  I knew when i saw the notification that you completed another challenge that i won't be disappointed and you prove me right again. I think you did an awesome job on this one. Everything is working like expected, there were no hiccups or anything strange to happen. I tried it on a 27" Desktop and a 6" Smartphone. Restarting browser to continue where you left of is working like expected.

  So let's get started with some of the minor improvements in my opinion:

  1. when you are playing with the mouse i would love the see the cursor:pointer on the buttons. It is such a minor thing but in my opinion it is definitely needed here
  2. on the intro screen where you are choosing your opponent i would love to see marginally taller buttons (CPU vs PLAYER), it is slightly awkward to hit it on the smartphone device
  3. when your opponent is CPU just some delay to give impression that CPU is calculating his next move, to be more immersed in the experience of a game

  Looking forward to see you next challenges!

  Cheers, Miran

  Marked as helpful

  1

  This comment has received 1 replyShow
• 

  Grégory Locigno•50

  @WorkablerySubmitted about 2 years ago

  I had a lot of fun with this challenge! The HTML / CSS part was quite easy but the Javascript part took a lot of work, it was not easy.

  Thanks to the two people who helped me with the radio button. Without them, I would never have made it!

  If you could give me feedback on my code, that would be great!

  

  Tip Calculator

  1

  10
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Greg,

  first of all congratulations on completing this challenge!

  Using it for a couple of minutes i encountered some problems along the way and i will document it here.

  Starting from top to bottom:

  1. it is possible to enter non-numerical characters into inputs
  2. when i enter first character in the "bill" field there is a validation message in the "number of people" field, not necessary in my opinion
  3. if you enter something in the "bill" and "people" fields there should be a calculation for "total / per person"
  4. if you select "10%" from the predefined values and enter any number in "custom" field 10% is left as active
  5. when all 3 steps are populated ("bill" = 100, "tip" = 10%, "number" = 1) and you want to change the number of people, only "tip amount" is changed and not "total"
  6. if i enter 2 persons, and 200$ next nothing is happening, no calculation is done

  Cheers, Miran

  Marked as helpful

  0

  This comment has received 1 replyShow
• 

  shan1y•190

  @shan1ySubmitted about 2 years ago

  Added authentication and authorization but haven't made a bookmarks table in MySQL so that it only shows bookmarks of whoever logs in. Future implementation will include this

  

  BestFlix

  1

  21
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi shan1y,

  i would like to take a look at your project as it looks quite interesting but am stuck at the login page, did you add some dummy accounts for demo purposes or i need to create account for this? I tried looking at the github repo Readme file but couldn't find anything.

  Thanks, Miran

  0

  This comment has received 1 replyShow
• 

  Kostya Farber 🧟‍♂️•220

  @kostyafarberSubmitted about 2 years ago

  A fun challenging project!

  Enjoyed using loops in SCSS and using some JavaScript. This was a suitable layout to use a grid.

  Had some trouble getting the image to scale up in the footer on the mobile design so if anybody has some tips would be much appreciated!

  Thanks for checking it out.

  

  Launch Countdown Timer HTML/CSS, JavaScript, Responsive

  1

  20
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Kostya,

  congratulations on finishing this challenge.

  Regarding footer image you could do couple of things depending which route you want to go. Analysing the design you can calculate how much of vertical space this footer image takes and plan accordingly with markup and CSS.

  For example, if i remember correctly footer takes 25% of viewport height so you could create a grid or flex container with 2 rows and set top row for 75% and bottom for 25% of vh. That way you will always get perfectly sizes footer space. Next you could center the icons with display: grid; place-content:center; on the footer itself. Next step is to scale the background image. You could do something like this.

  background-image: url(pattern-hills.svg);
  background-repeat: no-repeat;
  background-position: 50% 0%;
  background-size: cover;
  

  This way you are telling the image to not repeat itself, that to be positions always in the center on X-axis, and on top on the Y-axis, and last to stretch and cover all the available space.

  So final footer styling would look something like this:

  footer {
      background-image: url(pattern-hills.svg);
      background-repeat: no-repeat;
      background-position: 50% 0%;
      background-size: cover;
      display: grid;
      place-content: center;
  }
  

  One more thing i've noticed is that when the numbers are changing you are getting minor shifts in the layout. You can fix it by adjusting the width tiny bit just to make sure that wider characters can fit in the design. Best way would be in my opinion to set width: 3ch; which is character width unit. If you are interested you can find more info about it here

  Keep coding! Cheers, Miran

  1
• 

  Felipe da Costa Malaquias•160

  @FelipeDaCostaSubmitted about 2 years ago

  Feedbacks welcome. Had a really hard time adjusting element positioning due to the varying size of the advices. How should I deal with this kind of situation?

  

  Advice Generator App

  1

  10
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Felipe,

  congratulations on completing this challenge!

  I suppose you are talking about positioning button element and if you are the solutions is pretty simple actually. All you need to do is get rid of height declaration on .advice-card container, that way you are making height more fluid and based on length of the text. Now your button is nicely positioned on a bottom edge of the container and all you need to do is push it (transform) somehow to the bottom even more. You can do that by transforming it on a Y-axis with: transform: translateY(50%);. That should fix the problem you are having.

  Keep coding!

  Cheers, Miran

  Marked as helpful

  0
• 

  Mok Wang Quan•120

  @mokwangquanSubmitted about 2 years ago

  Is there any part you think I can improve on?

  

  Planets fact site

  1

  10
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Mok,

  first of all congratulations on completing this challenge!

  I think your solution looks great, i really like the slider feel of the page when you move between planets. You did a great job with this project.

  Like always there are things that can be improved so let's get started.

  Desktop:

  1. on screens larger than 24" there is a lot of white-space bellow the content, i think that can be improved with some calculations regarding available height etc.. to create more balanced look
  2. also on Venus page, third tab "Surface Geology" has somewhat longer text and shifts the whole page up and down when you toggle between other tabs on this page; same experience can be seen on other pages aswell
  3. below 1200px of resolution when the navigation wraps below the logo after the transition ends page shifts from the top for around 20px or so, it seems like when animation is active whole thing is shifted up and when it finished it goes back down for some reason

  Mobile:

  1.mobile navigation when active is placed near the top of the page, needs some shifting down because right now if covering the logo area a bit,

  .el-dialog__wrapper {
      ...
      height: calc(92vh + 4px);
      ...
  }
  

  needs to be

  .el-dialog__wrapper {
      ...
      height: calc(92vh - 27px);
      ...
  }
  

  at least on my machine.

  Other than that like i said earlier it is great attempt and nothing major to add here.

  Cheers, Miran

  Marked as helpful

  1

  This comment has received 1 replyShow
• 

  Bachar_elkarni•160

  @Diamo-BSubmitted about 2 years ago

  This is the first project that made me feel actually cornered like hell, can someone pls gimme some pieces of advice especially in the responsivity part of the CSS code? I'll really appreciate it. Happy Coding :D

  

  e-commerce product page

  1

  50
  

  Miran Legin•680

  @miranleginPosted about 2 years ago

  Hi Bachar,

  first of all congratulations on completing this challenge!

  At first everything seems to be working fine but if you scratch the surface a bit you will find some things that can be improved. Let's get started!

  1. when you click the + sign multiple times you will notice that the button is slightly changing position left/right. this is because every number has different width so the whole thing is slightly shifting from left to right. let's assume that nobody is going to put more than 99 sneakers in the basket to our safe bet is to limit the width of the number container to 2 characters. in css that would be 2ch. more info on ch units also as this is not monospaced font more info on monospaced fonts we can be safe and put 2.25ch and also align that number to the right hand side with text-align: right
  2. another example with shopping cart, if you add more than 1 item when you want to remove article from the card you need to remove it 1 by 1, i'm not so sure about this practice and i would suggest to remove all of the from the cart unless otherwise is stated in the challenge brief
  3. when you open the modalbox with images you will see that icons are not aligned properly in the circle, that is only 1 part of the problem, another is that better practice would be to create button elements that you can style with background-images and attach click event listeners on button itself, also remove onclick attribute from markup itself
  4. menu-icon also has inline eventlistener so you can follow same advice as for buttons in a modalbox
  5. on mobile view when you open the menu if you inspect it in the devtools you can see that categories div is much wider than the body itself. that is because you are working in content-box model which calculates your width: 100% and adds padding: 1.5em 1.5em so you end up with 100% + 3em which is wider than 100% obviously. to fix that you can add

  /* apply a natural box layout model to all elements, but allowing components to change */
  html {
    box-sizing: border-box;
  }
  *, *:before, *:after {
    box-sizing: inherit;
  }
  

  to revert to border-box model which is much easier to work with more info on box-sizing

  Keep coding!

  Cheers, Miran

  Marked as helpful

  2

  This comment has received 1 replyShow