Skip to content
OverviewSolutions
7
Comments
176
Ivan
@isprutfromua

All comments

  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Good job!

    I have some suggestions for improvement

    • Close the picture by clicking on the background

    • It would be good if the detail page occupied the entire height of the screen, no more

    • The bottom panel twitches when you open the details page

    • Why do you need the index and [index] folders, what is the difference between them? I think folder components would be enough

    • instead of the slide-out.ts animation you could use the built-in fly animation with the value set to y

    I hope my feedback will be useful to you

    Good luck

    1
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there

    Please note the variable declarations. It is better not to use var, it will also be better if you declare the types of these elements.

    var intereactiveNumber=document.querySelectorAll(".numbers")!;
let mainDiv=document.querySelector('.main')!;
var submitBtn=document.querySelector('.sb-btn')!;
let updateRatingDiv=document.querySelector('.ratingText')!;
let ratingDiv=document.querySelector('.rate')!;
var ratingPoint: HTMLInputElement;

    It is better not to use any type, especially since you will always have an event target.

    element.addEventListener("click",(e: { target: any; })=>{

    You don't get an input element here, it's just a number

    let targets=e.target.dataset.num as HTMLInputElement;

    that would be enough

    let targets: number = +e.target.dataset.num

    In general, I'll recommend to repeat Javascript Basics Concepts

    REgards

    0
  • Shawn Lee•560
    @OGShawnLeeSubmitted over 2 years ago

    Hey what's going on? This one was built with the built-in Svelte transition and state management api so there was no need to install any external library.

    1. I had to add and remove margin to the main element to keep the job listings in the same place whenever the filters section appears and disappears. I didn't use absolute positioning for the filters section because if it grows vertically it would not push the list down. Do you guys know of a cleaner solution?

    Thanks for viewing/replying. Have a great day!

    Fully Animated with State Management Job Listing filtering app!

    1
    Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there

    About your question: you can use an empty block with a minimum height, or do the same with the grid areas.

    Regards

    1
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there.

    Your work is very similar to the design, it's great!

    But, I think you should make more use of the framework. Here are some of my tips:

    • move the cards to the data and use the loop each .. as. The data can look like this:
    [{stat: '10k +', content: 'companies'}]
    • You can also create individual components. For example - an image

    I hope my feedback will be helpful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    1
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there! Good job

    I have some recommendations for you

    • you need to stretch the background with background-size: cover

    • in my opinion, the calcTimer function is redundant. Instead, you could use the following construction:

    let diff = dateToLaunch - today;

function calculateTimer() {
.......
}

window.addEventListener('DOMContentLoaded', () => {
  const timerInterval = setInterval(calculateTimer, 1000);

  if (diff <= 0) {   
    clearInterval(timerInterval)
  }
});
    • In each interval tick you declare new variables with functions, this reduces performance
      const days = String(Math.trunc(time / (1000 * 60 * 60 * 24))).padStart(2, 0);
  const hours = String(Math.trunc((time / (1000 * 60 * 60)) % 24)).padStart(2, 0);
  const minutes = String(Math.trunc((time / 1000 / 60) % 60)).padStart(2, 0);
  const seconds = String(Math.trunc((time / 1000) % 60)).padStart(2, 0);

    I would make calculations in separate constants. The date calculation would look like this:

    const DAYS = 1000 * 60 * 60 * 24;
....
const left = {
  DAYS: null
  .....
}
...
calculateTimer() {
....
left.DAYS = Math.trunc(time / DAYS)

daysValue.textContent = days > 10 ? + days : `0${days}` ;   // use ternar because padStart is experimental
}

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    0
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Great work!

    I have a few comments and suggestions:

    • first of all - test your work yourself. If the top of the text is full of text, it will not be visible. Try it yourself. It is better to avoid fixed sizes where content can change

    • is an extremely unreliable selector

    .content section button

    • please set an unique class for the button. also i would add a smooth background color transition when hovering

    • It is better to use such a tag structure

    article
    header
        section
    footer
        section
        section

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    1
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Good job.

    I have some suggestions for improving css code:

    • declaring variables looks pretty ugly. You need to work on naming.
    
:root {
    /* ### Primary */

--very-dark-blue: hsl(233, 47%, 7%); /*(main background)*/
--dark-desaturated-blue : hsl(244, 38%, 16%); /* (card background) */
--soft-violet: hsl(277, 64%, 61%); /* (accent) */

/* ### Neutral */

--white : hsl(0, 0%, 100%); /* (main heading, stats) */
--slightly-transparent-white-main-para : hsla(0, 0%, 100%, 0.75); /* (main paragraph) */
--slightly-transparent-white-stat-head : hsla(0, 0%, 100%, 0.6); /* (stat headings) */
}

    In my opinion, it would be better like this:

    :root {
    --very-dark-blue: 233, 47%, 7%;
    --dark-desaturated-blue: 244, 38%, 16%;
    --soft-violet: 277, 64%, 61%;
    --white : 0, 0%, 100%;

    --color-text: hsla(var(--white), 0.75);
    --color-heading: hsla(var(--white), 0.6);
    --color-accent: var(--soft-violet);

    --bg-body: var(--very-dark-blue);
    --bg-card: var(--dark-desaturated-blue);
}
    • Your effect does not match the design. Instead, you could use a pseudo-element with a gradient background.
    filter:brightness(80%);
    • also, I would advise you to read about css methodology. This will help you better organize your code. In my opinion, this design has only two elements - a card and a picture. That is, in css it will be two components - .card and .image. Further from them you build a tree structure
    card
    -  card-content
        - card-header
        - card-text
        - card-stats
            -card-stat
            .....

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    1
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Good job!

    I have some suggestions for improving the result

    • Always try to use native elements. It will be better if you use a button element instead of a div. So it can be controlled with the keyboard

    • You can also show the menu when you hover the button. With css:

    .user__share-icon:hover + article-card__user__share-socials {
transform: translateY(0);
opacity: 1;
}

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    0
  • GabFerreira•240
    @Gab-FerreiraSubmitted over 2 years ago

    I had some problems. My icons from fontawesome are not showing correctly, they are white squares. I couldn't put style to a input type button so I created a div to be the button of the form.

    Thank you for your attention and give me advices if you want :)

    error check with javascript

    2
    Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Your decision looks good, but there are some corrections:

    • Icons are still not displayed. In my opinion, it is better not to rely on third-party libraries, so I always use svg sprite. The icons are always on the server and it will not be such a surprise that a third-party library did not work.

    • making the button an extraneous element is a bad practice, why don't you just use the input type submit or the button type submit?

    • You also need to add an alternative description for the picture. Or hide it with aria-hidden = true.

    • Your site is perfectly responsive, but he lacks padding top

    I hope my comment will be useful

    All the best

    Marked as helpful
    0
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Good job! Your solution looks very similar to the design.

    I have some suggestions for improvement:

    • set text-transform: uppercase for section headers

    • I think it would be better to import page components out of a config

    import HomeView from ....
....

component: HomeView
    • For a better UI, you can add smooth transitions between pages. For example:
      <transition name="fade">
    ....
  </transition>
    • I think you could simplify your data file. You could export each page individually, instead of importing an entire data file into single file components.

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    0
  • John Idenyi•250
    @JohnIdenyiSubmitted over 2 years ago

    Hello, Frontend community! I had difficulty trying to use an overlay for the image, although I think I managed to work something out. Open for suggestions!

    Stats preview card component

    1
    Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there. Good job! I have a few comments and suggestions:

    • your card starts to break at 1337 pixels, which is not good. It would be better if you added these styles
    .text-container {
  /* padding: 60px 90px 0 60px; */
  padding: 60px 90px;
}

.image-container {
  /* height: 100%; */
}

.image-container img {
  height: 100%;
  object-fit: cover;
}

    So the picture fills the entire space vertically and horizontally.

    • It doesn't make sense to set additional classes for statistics wrappers. They are not unique, so it would be better to use a table element in this case.
    <div class="companies">
...
</div>
<div class="templates">
...
</div>

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    0
  • Ivan•2,630
    @isprutfromuaPosted over 2 years ago

    Hi there! Your solution looks great.

    On my opinion you can improve this things:

    • try to avoid overwrapping. You use 4 elements as a card wrapper : main > section > .container > .cards-container

    Instead of this you can simply set to the body these styles:

    display: grid
place-items: center

    It will be enough.

    • Instead of using a svg image, you can use a background image. You could convert svg to background with SVG encoder.

    • it's a bad idea to use the aside element as a card wrapper. It is more suitable for panels. You can use section instead.

    • There is no reason to complicate selectors here. You can simply use single .stats selector.

    .more-details .stats

    I hope my feedback will be useful.

    Good luck and fun coding 🤝⌨️

    Marked as helpful
    1