Skip to content
OverviewSolutions
28
Comments
8

Matthias Nilsson

@RalfiSlaskSweden980 points

Aspiring frontend developer that likes to explore and find solutions to problems!

I’m currently learning...

Vue

Latest solutions

View more solutions

Latest comments

  • Matthias Nilsson•980
    @RalfiSlask

    Hello!

    Regarding star image

    Easiest way to fix the issue is to have the star image inside of a div and then center the image using flexbox. That way by seperating them it is much easier to not get any unwanted behaviours.

  • Matthias Nilsson•980
    @RalfiSlask

    Hello!

    Javascript feedback

    Variable Names

    Change the name of btn to buttons as it targets all the buttons and not a single one, dont be afraid of writing larger names as it is important for other programmers to be able to understand your names. For example bck can be really hard to understand what it means.

    Selectors

    Have all the selectors at the top of your file.

    Using var instead of let and const

    I would suggest using the more common standard let and const for declaring variables as var is a bit outdated. It is generally better to use let and const instead of var because of their nature in scoping and hoisting which can lead to some bugs if you are not familiar with its nature. Also how you declare your functions changes the hoisting, arrow functions will not be hoisted where function() will be.

    Let and const are block scoped and var is function scoped, var will also be hoisted which means that it bubbles up to the top of the file. Block scoped means that you can only handle the variables inside blocks for example if else where function scopes means that you can reach variables everywhere inside a function.

    const checkScope = () => {
  console.log(varInsideBlock) // will print undefined 
  cause of hoisting this becomes the declaration var varInside; 

  if(1 === 1) {
    var varInsideBlock = "function scoped";
    let letOrConstInsideBlock = "block scoped";
  };

  console.log(varInsideBlock) // will print 
  console.log(letOrConstInsideBlock ) // will produce ReferenceError
};

checkScope();

    Array methods

    Use array methods instead of for loops when you are working with arrays or NodeLists. For loops are better if you are working with numbers etc.

    buttons.forEach(button => {
  button.addEventListener("click", function() {
    button.classList.toggle("selected");
    result = button.innerHTML;
    span.innerHTML = result;
  })
  button.addEventListener("blur", function() {
    button.classList.remove("selected")
  })
});`
    Marked as helpful
  • Matthias Nilsson•980
    @RalfiSlask

    Hello nice work!

    I have a few suggestions and tips.

    HTML

    Change semantics of your elements so instead of <div className="footer></div> use <footer></footer>. Use only section if it is necessary.

    Javascript

    Maybe use ternary operators for simple if statements, it is easier to read as it only takes up one line.

    instead of:

    if (isDiscounted) {
     newPrice = newPrice * 0.75;
 }

    use:

    newPrice = isDiscounted ? newPrice * 0.75 : newPrice

    React

    Make use of react by creating components that can be reused if necessary so you dont have to repeat code. The most obvious example is in the footer, where you have three items that could be used in a single component.

    If you instead create a component like this and make use of the text as a prop:

    const FooterFeatureItem = ( {text} ) => {
  return (
    <section className="check-section">
        <img src="./icon-check.svg" alt="" />
        <h3>{text}</h3>
    </section>
  )
}

export default FooterFeatureItem

    When you then have this component you can either use it by just writing it like this:

    <section className="bottom-section">
       <FooterFeatureItem text="Unlimited websites"/>
       <FooterFeatureItem text="100% data ownership"/>
       <FooterFeatureItem text="Email reports"/>
 </section>

    But a better and more dynamic way is by first creating an array of objects:

    
const footerFeaturesList = [
        {id: 1, text: "Unlimited websites"}, 
        {id: 2, text: "100% data ownership"}, 
        {id: 3, text: "Email reports"}
]

    And then use the map method over this list to create the components:

     <section className="bottom-section">
     {footerFeaturesList.map(feature => <FooterFeatureItem key={feature.id} 
     text{feature.text}/>)}
 </section>
    Marked as helpful
  • Matthias Nilsson•980
    @RalfiSlask

    Hello!

    Possible Improvments

    Styling

    • Maybe remove spellcheck for the inputs.
    • Have hover and focus effects on both the inputs and the button.
    • Inputs should not be filled in from the start, only have the placeholders.
    • Have better spacing between the inputs, button. Tip is to use flexboxs gap!

    JS/Functionality

    • Add error handeling when the inputs have not been filled in, are strings instead of numbers etc. You can use regular expressions or other ways of checking this.
    • The confirm button should handle the functionality, you can for example create a list as a state that keeps track of all the input values in each input.

    React

    • Organize your code into smaller components instead of having everything in the App component, that way if you want to change the application and add for instance another input you can reuse the input component etc. Then in those component pass down props.
    Marked as helpful
  • Matthias Nilsson•980
    @RalfiSlask

    Hello!

    Mainly i would suggestion having a better file and folder structure which in turn will make it easier for both you and pleople looking through your code.

    First add the fonts to its own folder, for example assets/fonts/overpass.

    Second i would recommend to use external css stylesheets and javascript files as it is easier for the browser to read and also for others reading through your code and this is a more common standard then having everything in a bloated html file.

    Maybe adding some hover effect on the buttons would add a nice touch to the project aswell, dont know if that is in the figma description.

    Lastly, create your own readme file and take a screenshot of your project (firefox) and add to your Github profile. Instructions should be in the original readme.

  • Matthias Nilsson•980
    @RalfiSlask

    Hello!

    Here are some things i think you can improve in your project:

    CSS:

    • Would suggest having the height 100% in body, html and main so that your background color covers the whole page.

    • Have spacing between your selectors so it is easier to read.

    JS:

    • Instead of using an XMLHttpRequest try using the newer more modern way using fetch.

    • Object deconstructuring. Instead of having to type out each name like data.slip.advice // data.slip.id you can write as const { advice, id } = data.slip which is the same as creating two variables const advice = data.slip.advice and const id = data.slip.id.

    • Template literals. Instead of writing text.textContent = '"' + advice.slip.advice + '"';. You can use `` and inside these you can type your variables using ${variable}.

        try {
       const response = await fetch("https://api.adviceslip.com/advice")
       const data = await response.json();
       const { advice, id } = data.slip;  
       text.textContent = `"${advice}"`; 
       adviceId.textContent = `ADVICE # ${id}`;
    } catch (error) {
        console.log(error)
    }
}

fetchData();

button.addEventListener("click", fetchData)
    Marked as helpful
View more comments