Clock app / SCSS, Flexbox, JS

Community feedback

• 

  Justin Morgan•95

  @justin-m-morganPosted about 3 years ago

  Hello Tereza,

  Great job on this!

  The heading issue is an interesting one. The design file suggests heading levels but this is not in accordance with best practices. In reality, the styles of the "headings" (font weights, spacing, line-height, etc) should not be tied to a particular element. Consider instead creating "h1" - "h6" classes (or call them something different if it makes more sense) and then reassign the actual elements so that they conform to that suggestion of only dropping by one level at a time (or use a non-heading element instead).

  Your second issue is more open ended. In general, you want to have the main container only be the height of the window and either hide or conditionally add the content that appears below the window (I call it the "more info panel"). In my case, I made a container that was 100vh tall (that's 100% of the viewport) with overflow set to "hidden", and made my inner content in a container that slides upwards into view (and the top content slides out of view at the top of the container). This has its own quirks that need to be worked around (ex, on iOS Safari, the bottom bar overlaps the bottom ~5% of the window).

  Hope this helps.

  Marked as helpful

  2
• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. Layout looks fine and the time is accurate I think. Mobile layout is great as well.

  Justin already gave feedbacks about your question, just going to clear or add some as well:

  • "Heading levels should only increase by one" just means that when you use for example a h4 make sure that h1, h2, h3 are present, "before" it. Always make sure that when using heading tag, the level before it should be present as well but before the current-heading-tag you will use.
  • I think it should be a large dropdown, since like what it said, "more" and its a button it should expand something to show.
  • The refresh button's svg element should have aria-hidden="true" on it , so that the refresh icon won't be picked up by assistive tech.
  • On the blockquote you should put the name of person inside it since the text and the person is just one component of it.
  • Just an addition, it would be great to have aria-live element on this, so that for example, every 10 minutes, it announces the current-time, just an extra idea.
  • If you inspect it on dev tools at the bottom, a second vertical scrollbar appears because of overflow-x: hidden, this makes the overflow-y: scroll or auto I think, creating a second scrollbar.
  • Also, there are lots of absolute element on this one, if you try to hover on the body on dev tools, you will see that it doesn't logically captures the whole layout, since those elements are out of the flow.
  • Avoid using height: 100vh on an container elements, this makes the element's height be limited to only the current viewport's height, I think this creates the bug of what I just stated above this line, about the layout not capturing the whole content. Instead, remove it or replace it with min-height: 100vh, this will take full viewport and it will expand if it needs to.
  • width: 100vw as well is not idea since this just creates extra spacing because it doesn't account the space of the scrollbar, width: 100% works just fine or if the element is block element, then you don't need to set the width of it.
  • I wouldn't use heading tags on the numbers below as well as the location. I am referring to the more or less section below. The CURRENT TIMEZONE, DAY OF THE YEAR those being heading tags are meaningful enough to make sure that users will know the next text will be the information regarding the heading tag.
  • Lastly, background-position: fixed is really bad, it looks fine but it makes the page slow. Try scrolling up and down, do you notice some spikes or small pikes? That is the effect of it, try removing it, it is smoother right? That is why, I avoid using it and not really recommend using it.

  Aside from those, great work on this and for the worldtimeapi

  Marked as helpful

  1

  ---

  

  Tereza•605

  @sirriahPosted about 3 years ago

  @pikamart Thank you for your reply. I have some questions:

  • I did not set position absolute to any element, except the day_details - I change it now, because it was not necessary and I forgot to remove it after I change the whole concept. You have said that there is a lot of absolute elements - where?
  • You wrote about the scrollbars that are appearing after you open the developer tools, but I cant see any. What device or system or browser do you use?
  • I understand the headings - but I worked according to the instructions in the figma file. It is little confusing that it is designed to cause these warnings. :) I try to contact Matt at the Slack.

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  @sirriah Hey.

  • Oh yes, sorry for that one, the absolute element, since I saw the day_details having it, I initially thought that there inside it are other absolute elements. My bad for that one :>
  • For the scrollbar, try inspecting when your dev tools is at the bottom, then maybe adjust the dev tools height, you will notice that there is an extra scrollbar because of the overflow-x declaration on the body tag.
  • Oh, design files have those if there are headings? Didn't know about that, but still, like it was said before.

  0
• 

  Marlon Passos•940

  @MarlonPassos-gitPosted about 3 years ago

  Good job Tereza, I'll try to add something beyond what the guys already said

  When we are in a city that the name has 2 words or more it adds a "_" between the words, if you took that away it would be better, see: https://prnt.sc/1t7ggns

  I do the following

  let name = details__timezone
  undefined
  name.innerText = name.innerText.replace(/\_/, ' ') // <-- add that your code in the element
  'America/Sao Paulo'
  

  It's much better: https://prnt.sc/1t7gsbd

  Marked as helpful

  0