Expenses Chart Component

Community feedback

• 

  P

  visualdennis•8,375

  @visualdennissPosted 6 months ago

  Hello Hira,

  To answer few of your questions: Pseudo-classes: define a special state of an element (e.g., :hover, :first-child). Pseudo-elements: style specific parts of an element (e.g., ::before, ::first-line).

  For mobile and general avoid giving fixed widths like 35px for the individual bar. they should be like 100%, and you can use display:grid for the #bars. E.g.: display: grid; grid-template-columns: repeat(7, 1fr); /* Create 7 equal columns */ and remove the fixed widths from the grid-items. And also add a max-width if you want for the bars so that they don't get too wide.

  as for the JS part. couldn't look in depth but, e.g. const popups = document.querySelectorAll(".popup"); is declared but not used since it is locally scoped inside the event listener func of DOMContentLoaded. Also I think instead of all those mouseOut event listeners, a simple :hover in CSS could suffice. Leading to cleaner and concise code.

  Hope this helps and keep up the great work! :)

  0
• 

  Hira•130

  @hirashabeerPosted 6 months ago

  salam @0xabdulkhalid,, Could you please spare some of your precious time to answer all these 5-6 questions of mine in retrospective.....and check why the logo and favicon-32x32.png is not displaying in my site while i have added the images in my repository .. Thanks in anticipation

  0

  ---

  

  Abdul Khaliq 🚀•72,560

  @0xabdulkhaliqPosted 6 months ago

  Wa alaikum salaam warahmatullah! @hirashabeer

  I have read all your questions and inspected your code, after inspection i have found that you just overused css rules. You can improve it by though!

  For example, You don't need to use width and height with vw & vh units for components. It's the reason why the component is not responsive for all sorts of devices.

  You can wrap the header and main with a div element and remove these following styles,

  header {
    width: 43vw;   // instead use - '100%'
    height: 23vh;
    margin-top: 30px; // instead use 'margin-bottom: 1.25rem'
    padding: 0 30px 0px 30px; // instead use '1rem 2rem'
  {
  
  main {
      height: 100vh;
      width: 90vw;  // use - '100%'
      margin-top: 30px;
  }
  

  ---

  • You need to remove width and height rules from media queries also (max-width: 375px).

  ---

  • Looks like you followed up Desktop First Approach, so that you designed the site for Desktop then you adjusted it for Mobile devices. Trust me this approach itself have a lot of problems and cons.

  ---

  • Instead you try Mobile First Approach, this approach is better in a way that instead of ignoring the responsive CSS to be added in the end, you have to work on it first, which gives you a responsive site from the beginning.

  ---

  • Regarding the Logo & Favicon; During inspection i found that the Favicon is working fine no issues with it, but the Logo which want to be displayed on the header is not shown. That's simply because of the source path. You can simply fix it by adding a period (.). Eg: src="./images/logo.svg"

  ---

  • Additionally you also need to place the footer component without using margin for .attribution. Using margin is not a good solution and also the static layout will cause alignment issues with chart component. Because we need to place it center both horizontally and vertically.

  ---

  • So you can add position: relative to body and these styles to footer,

  footer {
    position: absolute;
    bottom: 1rem;
  }
  

  ---

  • In JavaScript i noticed that you have selected all popups,

  const popups = document.querySelectorAll(".popup");
  

  • But you never used it, instead you select each popup from bar element's previous sibling object.

  ---

  • Actually you don't need mouseover and mouseout events to trigger those popups, Instead you simply add the values for popups to bar elements which can be encapsulated on data attributes. Then create pseudo elements using :before or :after. Once you have done it you can access it later using attr() function in css.

  ---

  • At last, can you explain a little bit about this code? For what reason you have added this code? - I think we don't need it.

  //   const today = new Date().getDay();
  
  //   if (index !== 6) {
  //     index + 1 === today && bar.classList.add("highlight");
  //   } else {
  //    today === 0 && bar.classList.add("highlight");
  //   }
  

  ---

  Hope this helps you, looking forward to see your Growth!

  Marked as helpful

  1

  

  Hira•130

  @hirashabeerPosted 6 months ago

  Thankyou so much for your time and help @0xabdulkhalid ! The code you asked about at last is what I wrote for making the current day bar blue but this was not working so I commented out to ask the reason... Secondly can you please some resources to learn about the semantic html, accessibility, responsiveness techniques etc. And one last question pleaseee I am thinking of learning react share with me any useful resource

  0

  

  Hira•130

  @hirashabeerPosted 6 months ago

  Also @0xabdulkhalid I didn't get your this line You need to remove width and height rules from media queries also (max-width: 375px). ... Secondly in `header { width: 43vw; // instead use - '100%' height: 23vh; margin-top: 30px; // instead use 'margin-bottom: 1.25rem' padding: 0 30px 0px 30px; // instead use '1rem 2rem' {

  main { height: 100vh; width: 90vw; // use - '100%' margin-top: 30px; }` If I give it 100% it will consume full width of screen

  0