Raymart Pamplona

Latest comments

• 

  DeanFHardy•30

  @DeanFHardySubmitted about 3 years ago

  

  Advice_Generator API

  #fetch

  3

  42
  

  Raymart Pamplona•16,060

  @pikapikamartPosted about 3 years ago

  Hey, nice work on this one. The desktop layout looks good, however layout issues appears when resizing the browser from desktop view to mobile view as the layout is not responsive. Also, when you zoom out on your end, the components gets huge.

  Ivan already gave great feedback on this one, just going to add some as well:

  • Yep, when you code mobile first, resizing the browser using dev tools into a phone's size should always be done so that you'll really see the layout being shown into that size. Actually at first, I find it a bit confusing to do mobile first, but some challenges taken, i'm sure that you'll get the hang of it.
  • Never use position absolute a large container on your site, on your end, using it on the main tag. The reason for this is that, position: absolute removes the element from the flow, if the container doesn't have explicit sizes, it makes the container doesn't "capture" the child element. Try to inspect your site in dev tools, hover on the body tag, you'll notice that it doesn't have any size because its child element is being absolute. For this one, you can just remove all these stylings on the main tag:

  position
  top
  left
  bottom
  right
  margin
  width
  height
  

  Then to properly center the content, add these first on the body tag:

    align-items: center;
    display: flex;
    justify-content: center;
    min-height: 100vh; # makes sure that it has sufficient height
    padding: # add some to prevent component touching the sides of the browser
    place-content: center;
  

  Then again on the main tag, you can just add:

    flex-basis: 100%;
    max-width: 500px; # convert to rem and change the size depends on the design;
  

  This makes the component more responsive. You can add min-height on the main as well if you want if you like a more consistent sizing.

  • Now, if you follow those above, you will notice that some or lots of element are being out of place because those elements are using the same stylings on the main. Again, remove all those stylings I mentioned earlier on each element and let the main container handle their positioning. If you need to place elements, don't always jump to position: absolute this should be the last case you want. Try searching or maybe looking up other website's submission as well to get some ideas when you are coding :>
  • Do not use id attributes to target an element on your css, using id creates problem due to specificity, always use class so that it could be more manageable and reusable.
  • As you may know, an h1 is needed for every webpage, the h1 describes the main content per each page. On this one, since there are no visible h1 on the page, the h1 will be instead a screen-reader only h1. Meaning, it won't be seen visually, but it is there. Have a look at this simple fiddle that I have about screen-reader text. Comments are already included, but if you have any queries, just let me know okay.
  • Instead of h4, use h2 on the "Advice" text. When you are using heading tags, make sure that they are on the proper level, when you use h4, make sure that h1, h2, h3 are all present before the h4.
  • When you need to make a text capitalized, you don't write them as it is like "ADVICE" on the html, this will make screen-reader read the word letter-by-letter and not by word. Use lowercase on them and just use text-transform: uppercase on the css.
  • If you want to align those items inside the main tag in the center, you can add:

  align-items: center;
  display: flex;
  flex-direction: column;
  

  On the main tag.

  • Remember when using img tag, always add an alt attribute. When you don't include it, screen-reader will instead announce something different from the file path. So always include it.
  • Since the divider img is just being used as decoration, adding alt="" and aria-hidden="true" on it would be nice. This makes the img tag be hidden for screen-readers as they are not really meaningful content of the site, always use this when image are not informative.
  • Instead of input, using button would be much better and correct for this one since input are used inside of form right.
  • Using button on that one, I would suggest the .circle selector to be the actual button, just style it to be circular. Also, since there are no visible text on that button ( if you used ), you should always add a label-text on it on what the button is supposed to do. For example, you can add aria-label="Change quote text". This way, when user traverses the button using screen-reader, they will be notified on what this button does.
  • If you are new to some of these ideas, maybe adding more will be confusing right now. But some ideas to make the app more accessible would be, shifting focus on the quote after the button is toggled or an alternative, aria-live would be use on the quote so that it will be easier to maintain response.

  Those might be lot but you'll encounter them on your way. Just let me know if you need help and see if I can help. Again, great job for this one.

  Marked as helpful

  1
• 

  Anaz Anoiar•160

  @AnazAnoiar69Submitted about 3 years ago

  

  Four Card Feature Section

  #sass/scss

  4

  20
  

  Raymart Pamplona•16,060

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. The desktop layout looks really nice, the site is responsive and the mobile state looks really nice as well.

  Others already gave their feedback which is nice to see, just going to add some suggestions as well and don't mind me if I re-iterate some ideas mentioned already:>

  • For the scss part, you don't really need to use:

  body {
    .... other selectors
  }
  

  You can just directly target the selectors like .container, this way you reduce specificity. Also, if you like, you can search about bem convention. This will help you manage css selectors and create more generic classes. For example, you will have something like:

  .cards{
    &__container {}
    &__list{}
    &__item{}
  }
  

  This way, you can group them properly if you think about it.

  • Adding max-width on the body tag to prevent the layout from stretching. If you try to zoom out on your browser, you'll see that the layout stretches, adding max-width will prevent that, just make sure to add margin: 0 auto so that the body will be centered.
  • These two text:

  Reliable, efficient delivery
  Powered by Technology
  

  Are just one single phrase, meaning instead of using p tag for the first one, use a single h1 to wrap both lines and just add max-width on the h1 so that it will limit and create that 2 lines.

  • The overall font-sizes could be bigger, right now some text are small and they are smaller than the design as well.
  • Remember to only use a single h1 for a site. The h1 typically use on the hero heading like the 2 lines above on the site. So each of the card titles could be replace by just h2.
  • Each images could use an aria-hidden="true" attribute so that it will be totally hidden.

  Those only. Again, great job on this one.

  0
• 

  Achref KS•530

  @AchrefFastSubmitted about 3 years ago

  

  Used NextJs React CSS Grid , FlexBox, Mobile-first workflow

  #next#react#sass/scss

  1

  30
  

  Raymart Pamplona•16,060

  @pikapikamartPosted about 3 years ago

  Hey, really nice work on this one. Overall, the layout for both desktop and mobile state looks really great. The reply also works great as well the upvote and downvote.

  For some suggestions, here are some:

  • If you try to click the first upvote of the first comment, you can't point to it , I think it is because of the h1 being sr-hidden. You can add clip: rect(0 0 0 0); on the h1 to fix it.
  • For each of the upvote and downvote, currently it doesn't have any informative text inside it and + or - alone is not enough. What you can do is that, add an sr-only text inside it to which could be downvote comment and upvote comment, depends on where it is placed.
  • Also, I would suggest changing the html placement for each of the post. The post's body should be placed before the ratings so that when a user navigates on the component, they will traverse the user, the user's post before the rating. Because it would be confusing to first traverse the upvotes/downvotes when the content is not first reviewed by the user.
  • Adding an aria-live attribute on the element wrapping the counts so that assistive tech will announce the update to the user on what happened. You can add extra text if you want, an sr-only text. Also, you could change the span to just p tag.
  • For each of the person's img, since their name is already present, it would great to use their name as the alt value since it makes sense to do so.
  • Actually, if this were a real app, each person would be a hyperlink right. Maybe making them wrapped inside a tag? Each person's img and name. Could be right.
  • Whenever you wrap a text, always use meaningful elements and not div. Changing all those div that wraps a text should be replaced by p tag.
  • For the reply button, the arrow-svg should have an aria-hidden="true" attribute on it since it is only a decorative image.
  • Again for the reply button, adding aria-expanded attribute would make it more accessible. This way, when a user toggles it, assistive tech will inform the user that it shows or expanding something.
  • For the reply form, using the user's name on the user's avatar alt attribute would be nice.
  • Adding label for the textarea would be great. Remember to always add label for each input or textarea element. This way, when a user is using a different language, those label text will be changed as well since attribute's data is static, the placeholder won't be translated. The label will be sr-only on this one.
  • There is a confusing part on the textarea. Initially, it should be empty right because a user is still not typing anything, but since you are already adding the @_name on field, a user can press the reply to send that reply even if they haven't add anything on the textarea and it will be much confusing since it is using required and to be honest, I can't think of an approach on this one, so maybe just letting you know about this? :>>
  • When clicking now either the reply or cancel button it would be nice again to have an aria-live element or maybe you can add a toast-notification on this one. The toast's text-content will vary on the user's choice, it could be like successfully replied to {person's name} or could be like reply cancelled. Just remember to add an aria-live="polite" on the element so that it will be announced.
  • When toggling the delete button, it would be nice to set the focus to the modal. This way, user will immediately be informed on what is the content. Right now, if you use keyboard to toggle the delete, then tab again, the user won't traversed on the modal like what it should do. So when a user toggles the delete, add the aria-expanded attribute then shift the focus in the modal, so that if they tab again, they will tab inside the modal. Another feature to add as well is to have a trap focus inside the modal. Meaning they can only tab on the two options, this way the user won't be confused on where they are at now.
  • To be honest, there are lots of added aria-live on this one or some toast, for the successfully deleting a reply and adding a comment as well.

  Just in general, if there are lots of state in your app that requires to send an update to the user, an aria-live would be nice or sometimes changing the focus on the pop-up element is really nice.

  Aside from those, great job again on this one.

  Marked as helpful

  1
• 

  Kamasah-Dickson•5,570

  @Kamasah-DicksonSubmitted about 3 years ago

  

  Responsive Faq accordion secion made with CSS grid and CSS flexbox

  #sass/scss

  1

  50
  

  Raymart Pamplona•16,060

  @pikapikamartPosted about 3 years ago

  Hey, nice work on this one. For desktop layout, I think it looks fine, just needed for the proper font-weight to be used and also the svg's outline is overflowing, on the original, the outline should be cut. The mobile state looks fine as well.

  On your questions:

  1. Yep, working with images sometimes is hard specially when you need to position: absolute something but I think that you did great on this one!
  2. For the breakpoints, it depends sometimes on the design of the project that you are creating, sometimes it doesn't need to use breakpoints especially when you created the component really responsive. For me, when I use breakpoint, I typically go with 768px for the tablet and 1000px on desktop state, I always start now with mobile first approach. Sometimes this changes because like I said, it depends on the layout.
  3. You can if you want and there's nothing wrong with that. I use that when there is only a single component of the page and I want the whole body to occupy the full height.

  Here are some other suggestions for the site:

  • When including images on your site that acts only as decorative images, you should always use an empty value for the alt attribute and adding an extra aria-hidden="true" attribute on the img would be nice. On this one, the lady illustration should be using those attributes I mentioned since it is only decorative.
  • Just a remined, only use descriptive alt values for images that are really meaningful on the site and when adding the text, the text should not include words that relates to graphic such as illustration, image, icon since the img tag is already a graphic and no need to describe it as one.
  • Change those FAQ question from using h2 into using button or maybe just use a details element on this one. When creating applications or websites, if a component is interactive, always use interactive elements like button. I saw that you are using the div as a toggle for the content which should not be the case.
  • Those arrow-images should be hidden as well since it is only a decorative image. Use the above method I mentioned to hide it.
  • For each question's answer, those are only being hidden visually by the overflow and max-height but a user can still traverse those. If you need to hide an element, adding the visibility: hidden should be done and visibility: visible if it needs to be shown.

  Aside from those, great job again on this one. Let me know if you need any help.

  0
• 

  beshoy•80

  @beshoyyatefSubmitted about 3 years ago

  

  Flexbox

  2

  30
  

  Raymart Pamplona•16,060

  @pikapikamartPosted about 3 years ago

  Hey, nice work on this one. For the layout, on desktop view maybe adjusting the padding would be nice. If you look at the design, there is a fixed padding on the left and right side for each of the landmark elements, you can follow that to improve the ui. Some text as well got smaller after the hero section. For mobile state, I think it looks fine.

  Here are some suggestions besides Divine Obeten feedback:

  • For the the site-logo-link, always remember to add either aria-label or an sr-only text inside it so that a user will know where the link would take them. You do this when there are no text-content inside the anchor. For this one, you can use like aria-label="homepage" on the a tag.
  • Always add the site name on the site's logo because that logo is one of the meaningful element on the site. Use alt="Huddle".
  • You can include the site-logo-link inside the nav if you want since it is being treated as link. But to be honest, I think you can just replace the nav by just a div since the link/s inside is not much, the nav would be much better on the links inside the footer tag.
  • Don't use height: 100vh on an element, this will make the element's height not consistent. Try going into dev tools at the bottom, you will notice that the hero section's height got small because of this property. Instead, use min-height: 100vh so that the container will respond properly.
  • I would change those section tags into just div because section alone is not that informative as a landmark element. By navigating using screen-reader, when it traverses the section tag, it won't announce it as a landmark even if you are traversing it as a landmark compared to like main, header and footer. div would be fine^^
  • Don't use br tag to cut the text, you can just add max-width on the text-element.
  • For the hero-image, you can add an aria-hidden="true" attribute on the img tag so that it will be completely hidden for other assistive tech. You can this as well on the svg's that are used across the page.

  FOOTER

  • On the logo, you are trying to make it white right, adding background-color to img won't work. What you can do is that add the svg's code itself on your html, then change the fill property of either the svg or path I think to the color that you want and this will changed the svg's color.

  • Same goes again for the logo-link, use a text that describes where the link would take and use the site's name as the alt value.

  • Those 6 links are all related to one another and using a single ul to wrap them would be better and also, you can wrap that ul tag by nav tag since those are the site's navigational links.

  • Those social media links could be wrapped inside a ul tag as well since they are list of links.

  • Since you are adding a hover state for the social media, you are implying that those are interactive, hence wrapping those inside their own a tag would be better, added as well the sr-only text or aria-label pointing on where it would take the user.

  • Lastly, if you pushed an update to your solution, clicking again the generate report so that it will clear up some issue if you fixed it.

  Aside from those, great job again on this one.

  Marked as helpful

  1
• 

  Facundo•100

  @FacualemandiSubmitted about 3 years ago

  

  Form

  1

  10
  

  Raymart Pamplona•16,060

  @pikapikamartPosted about 3 years ago

  Hey, nice work on this one. The layout for the desktop is fine but it could use more width since it is small right now. Resizing the site, it works fine but the layout at 1000px below until mobile state, the layout doesn't respond well making each content of the main squished and at 600px to 800px, you will noticed that the text on the left are now being overlapped by the form. The mobile state looks great though.

  Here are some suggestions for the site:

  • You don't use a height on the body tag. I'm supposing that you want the body to take the full height right, instead of using height, use min-height so that whenever the content of the site gets bigger, the body will rescale to that size because min-height lets the element expand unlike height that gives a fixed size.
  • Instead of using width for each child of main, you can just use padding and maybe add a max-width on the body tag so that it will prevent the layout to just always take full width of the user's screen. Just make sure to add margin: 0 auto for it to be centered.
  • Replacing each section tag into just using div. Using section is not really informative at all as landmark because it doesn't give extra information about it when traversing using an assistive tech, unless you give an aria-labelledby to it pointing to an heading's id. div is much better to wrap contents.
  • Whenever you use an input tag, adding a label pointing to it will be great so that even if the user uses a different languages, text content on the site can change if they want to translate the text and values used in attributes like the placeholder or maybe an aria-label are not translatable. Adding the label on this with an sr-only class will be great.
  • For the submit-button, instead of using input, use button type="submit" to be more explicit.

  SUBMITTING A WRONG FORM

  • If such input is invalid, adding an aria-invalid to that input would be really great so that when the user traverses on it, they will be notified that the input is wrong.
  • The error messages for each should have an id. I would change the div into just using p tag since it is a text content. Each error messages should have an id to which will be referenced by the input using the aria-describedBy attribute. This way, the user will know what kind of error that they had made because of the error message. Each id should be distinct like id="firstName-error".
  • Lastly, to further improve the error-handling, you could add an element that uses aria-live inside the form. The text will vary according to the form's submission, it could either be something like form has been submitted, thank you or it could be form submission invalid, please check your first name, last name... inputs.

  Here is a sample block that uses the attributes to add each error:

  if ( input is invalid ) {
     input.setAttribute("aria-invalid", "true");
     input.setAttribute("aria-describedBy", id of the error message");
  } else {
     input.removeAttribute("aria-invalid");
    input.removeAttribute("aria-describedBy");
  }
  

  Here is a sample form submission of what I said. It is a simple accessible form that I created, the aria-live is implemented there as well. Let me know if you have questions about it^^

  • Lastly, addressing the responsiveness issue of the site. Try to check out the dimension that I said earlier and try to fix the responsiveness.

  Still, great work on this one.

  Marked as helpful

  0