Page Interactive-pricing-component, responsive HTML, CSS GRID

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, great work again on this another solution. The desktop view looks fine, it's just a bit pushed down, the h1 is bit bigger and the box-shadow for the form could be made more smoother. The site is responsive though and the mobile state looks fine as well.

  For some other suggestions, here are some:

  • On this one, you could just use div instead of header and put it inside the main tag since this is just a single main-content. Typically, a primary header consist of the site-logo, navlinks and maybe some controls.
  • Also, since this is just a one-column layout, you could just omit the display: grid on the body tag and just add like a min-height on the form or just add sizing on the elements if needed. Because I noticed that when you resize the screen's height when in dev tools at the bottom, the layout shifts a lot and reduces it's height.
  • For the slider, the text...page-views is not really suited to a label on that one since it is quite ambiguous on what does it mean and if you look at it, changing the slider changes the pricing, it would be nice to make those 2 announced. For this one, you could create an aria-live element that will announce the changes for the 2 items or use an output tag on those 2 items, the page-views and the pricing. You could either one.
  • For the input type='range', it would be nice to use either aria-label attribute or a sr-only label tag. Then use a meaningful text to be used as the value, it could be like pricing selection something like that or if you could make up more meaningful, that would be nice.
  • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span to wrap the text.
  • For the billing section, since the two are selection of billing choice, instead of using checkbox, you should use radio-buttons whenever there are selections where you can only select one. For that one, you should use a fieldset tag to wrap the radio-buttons, a sr-only legend tag to which contains a text that will describe the set of radio buttons, have 2 label it will be the month and yearly text. I don't recall if I use this markup before when I did this, but I have this simple snippet about theme-toggle if you like. Though it is different, the markup is the same on both. Let me know if you have queries about this one.
  • Also, this layout is basically a form where the start my trial text is the submit-button, therefore it would be nice to use form tag inside the main to wrap the component and use button type="submit" instead of a tag.

  Aside from those, great job again on this one.

  Marked as helpful

  0
• 

  Martha•510

  @Mtc-21Posted almost 3 years ago

  Hi merry christmas, thanks for your comments and the suggestion 👍, i have applied some of your suggestions, I haven't applied the fieldset tag change yet, but I understand you say the relation with sr-only.

  1