Skip to content
OverviewSolutions
12
Comments
22
Matt Seidel
@mseidel819

All comments

  • Matt Seidel•570
    @mseidel819

    the <button/> component comes with a border by default. To get rid of it, in your css file, you can add:

    button {
border:none;
}

    Or you can add the border:none to both your .circles and .submit classes. I'd pick the first option because it's less code.

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    Looks nice! I'm giving myself a crashcourse on Vue, and I'm glad I found your app to help guide me!

    I noticed that your validation doesn't account for string inputs. Some solutions might be:

    • turn the form inputs into type="number"
    • add a check in your validator to throw an error if input is a string
    • (more complex) find a way to validate the month input so it can accept 1-12 OR the month name. That could be tricky because of the variations: Jan, January, etc..

    as for your question "How do you build a form with inputs like these and avoid using div tags?", My intuition says its ok to use the <div> like you are. I've seen it done in credible tutorials. Does the HTML accessibility checker on here give you an error?

  • Matt Seidel•570
    @mseidel819

    Looking at your semantic markup--

    You want to consider screen readers. If you couldn't see your page, and a robot was describing it to you, would it be clear?

    First thing, I think you should change your <div class="add-to-cart-btn"> from a div to a button. That will signal to the computer that you have a button that is supposed to do something.

    For your header elements, think about how you rank the most important things. This Can line up with font sizes, but not always. In this case, I think the first thing that the viewer needs to know is the product name- this should probably be your <h1>. after that, the <h4 class="segment">Perfume</h4> could probably be <h2>. The prices could probbly end up being <span> elements. I don't think prices are considered semantic elements. (but don't quote me on that.)

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    Very nice! I think you could move the isSubmitted and selectedValue logic into App.js. That way, you can keep the two components more separated.

    return isSubmitted ? (<Ranking rankingHandler={rankingHandler} /> ) : (<ThankYou selectedValue={selectedValue} /> );

    I think this will clean up your files a little bit. As I type this, I realize you will have to use some props in your components, and some handlers for the scoring. I'm happy to talk more about it if you have questions or you're interested.

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    If you want to take this further, and practice rendering HTML inside your .js file, you could render your notification boxes dynamically. maybe you create a data.json file with all of the posts. something like:

    [
{name: "CodingTube",
img: "./images/codingtube.png",
content:"left the group Chess Club"
},
{name: "Matt",
img: "./images/matt.png",
content:"followed you!"
},
...
]

    You'll have to structure it in a way that works for you, and fits every use case for the different types of posts.

    Then you could target the div that contains the posts, use a map function, and insertAdjascentHTML to fill the boxes up. some pseudo-code:

    import Data from "./data.json"

const data = Data; (redundant?)
const commentContainer = document.getElementById('comments');

data.forEach(post => {
commentContainer.insertAdjacentHTML('beforeEnd",
`
  <div class="post">
        <img class="img" src=${post.img} alt="" />
        <div>
          <p class="text">
            <span class="name">${post.name}</span> 
            <span class="group">${post.content}</span>
            <span class="status not-read"></span>
          </p>
          <p class="time">${post.date}</p>
        </div>
      </div>
`
)
});

    I'm not using my editor for this, so please excuse any syntax errors....

    This way, you can reduce some of the repeated code. Also, if you wanted to add or remove, some posts (very similar to a real-world use case), you can just update the data file, instead of the HTML.

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    I think you could add a media query so that the height: 100vh on the body only appears on large screens. It is causing some issues with the box shadow overlapping content on mobile.

    Another little bonus: you could add cursor: pointer to your .continue-btn class to signal that the button is clickable(even though its not connected to anything for this challenge). also, you could add a :hover effect on the button for more interactivity. Maybe a slightly lighter color, or an opacity?

    .continue-btn:hover{
opacity: .8;
}
    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    This looks good! I like the way you implemented the "time posted" feature. I was just using dates, so I want to look into how you did that.

    When I upvote the 1st level comments, they aren't sorting. Try moving your sort function around to another place. From what I think I see, you initialize comments with a sorted array, but they never sort again after that first time. For example, what I did in my app was to sort the comments in the return function: {

           { comments_map.sort((a, b) => {
          return b.score - a.score;
        }).map(comment => {
          return (
            <Comment 
              key={comment.id + comment.user.username} 
              comment={comment}
              comments={comments}
              setComments={setComments}
              currentUser={currentUser}
            />
          ) 
        })
      }

    }

    now, each time this component re-renders due to an updated state, it will sort the comments accordingly.

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    I like how you handled the theme toggler. Mine ended up only being able to switch 1,2,3,1,2,3. I like that yours can go from 1 to 3 right away.

    The only potential help I can give is within your themes.js file. Instead of export { themeOne };, you can just add the export directly to the const declaration: export const themeOne = { "--main-text": "white", "--main-bg": "hsl(222, 26%, 31%)", "--toggle-key": "hsl(6, 63%, 50%)", "--toggle-bg": "hsl(223, 31%, 20%)", "--toggle-h": "rgba(249,108,99,255)", "--keypad-bg": "hsl(223, 31%, 20%)", "--screen-bg": "hsl(224, 36%, 15%)", ... All of your imports will get to stay the same, too.

  • Matt Seidel•570
    @mseidel819

    nice! someone shared this with me, so I'll pass it to you. In order to get rid of the little scroll button on your number input, add this to your css: `/* Chrome, Safari, Edge, Opera */ input::-webkit-outer-spin-button, input::-webkit-inner-spin-button { -webkit-appearance: none; margin: 0; }

    /* Firefox */ input[type="number"] { -moz-appearance: textfield; }`

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    looks good! I have a list of tiny things that I'll share:

    1. Instead of 2 short circuit statements in App.js, you can also write 1 ternary operator: {submitted ?<ThankYouModal rating={rating} /> : <RatingModal onHandleRating={handleRating} onHandleSubmit={handleSubmit} rating={rating} />}

    2. when you pass props to a component, something you can do (especially if you end up using props a LOT in a component) is to destructure the props at the beginning: `const RatingButton = ({onHandleRating, num}) => {

      const handleClick = () => { onHandleRating(num); } } in this example,propsgot destructured to{onHandleRating, num}. Now, you don't have to include props.` in front of them each time you use them.

    3. continuing in Rating.js, another way to handle the click event (i think) is to put onClick={()=>onHandleRating(num)}. The const handleClick is good practice though for when you have a large function that you need to use. This function is so small that you could do either.

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    Your solution is very clean and easy to read! Looks great.

    The only suggestion I have deals with the jsx that renders the tip buttons. Its totally unnecessary, but you might be able to save yourself some lines of code if you figure out a way to get rid of the repeated code. The only real differences for each button are the percent values inside. Maybe something like: const percentArray=[5, 10, 15, 25, 50]; ... percentArray.map(percent=>{ return ( <button onClick={() => setTip(percent)} className={tip === {percent} ? 'active tip-btn' : 'tip-btn'} > {percent}% </button> )

    so I added the percent values to an array that I could map over. Then I replaced the hard-coded values with the variable in the map function. (full disclosure: theres a 95% chance my brackets and parentheses are wrong in my example. upside down smiley face)

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    I love all the animations! I searched my account, and some weird stuff happened with the spacing on my profile info. search for random users and you'll see what I mean. I'm running out of time to work this morning, but I'll take another look after work today. Its an otherwise very nice experience!

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    It looks like every listing has the new and featured tags on them. Try creating a conditional for those tags so that it only displays if the json data marks those listings as "new" and "featured". Something like this:

    <div className='job-item-details-header'> <span><h4>{item.company}</h4></span> {item.new && (<span>New!</span>)} {item.featured && (<span>Featured</span>)} </div>

    What I've added is a short-circuit conditional that will only render each of those span elements if the corresponding value is true. You can also add this to the green border on the left of each job card. I believe it only shows up on a job if it's featured.

  • Matt Seidel•570
    @mseidel819

    I like your solution for light/dark themes! I'm going to study up on how you implemented that.

    Something you can do to organize your components is to put all of your styled components in their own folder, and export them out. so, instead of one SearchApp.js, you would have SearchApp.component.js, and SearchApp.styles.js.

    Some of your .styles.js folder might then look like :

    export const Container = styled.div display: flex; flex-wrap: wrap; margin: 0 auto; export const Header = styled.header width: 100%; `

    export const Wrap = styled.div`
    display: flex;
    justify-content: space-between;
    width: 100%;
`

export const Title = styled.h1`
    color: ${Props => Props.theme.titleColor};
    font-size: xx-large;
    transition: all 0.5s ease;
    
``

    and then in your SearchApp.component.js, you would import it all:

    import {Container, Header, Wrap, Title} from "./SearchApp.styles";

    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    This looks great! I like how you added the "rate again" functionality.

    Another way you can switch between components is very similar to what you've done, but it might involve less lines of code:

    Instead of checking for truthiness in each individual component, add a ternary to App.js. You'll also be able to get rid of the isSubmitted prop from each component.

    `<main> { isSubmitted ? <Thankyou rating={rating} handleSubmit={handleSubmit} /> : <Rating handleRating={handleRating} handleSubmit={handleSubmit} />

    <Footer /> </main>`
  • Matt Seidel•570
    @mseidel819

    This looks great! I think your code looks nicely organized, so my comments are just another way of doing it.

    I think you could get rid of your container.js and put all of that code into App.js. (and move your container.sass file accordingly, too). That way, you get rid of the middle-man by storing your state inside App.js. It is one less folder, and this extra code inside App.js wont be enough to clutter.

    When I did this challenge, I made a wasRatingSubmitted: false inside state. Then, I used that to determine which component to show. The submit button will then have an onClick that will change wasRatingSubmitted to true.

    {this.state.submitted===true ? <ThankYouState rate={this.state.submittedRating} /> : <ActiveState onRateSubmit={(rate) => this.handleRateSubmit(rate)}/>

    This way, you'll have both your wasRatingSubmitted and submittedRating inside state incase this project was to expand and now you need to know if the user submitted a rating somewhere else.

  • Matt Seidel•570
    @mseidel819

    I like the creativity on the button colors!

    a couple accessibility fixes:

    • Change <div class="continer"> to <main class="continer">
    • Change <div class="attribution" role="footnote"> to <footer class="attribution">
    • I think you can also get away with changing <h2 class="heading">How did we do?</h2> to an <h1> element.
    • on line 12 of your index.html, in , you can get rid of type="text/javascript".

    Other things you can look into:

    • when you hover over the number buttons and the submit, cursor should be a pointer. cursor:pointer.
    • when you hover over submit, the background should change to white, and the text should change to orange.
    Marked as helpful
  • Matt Seidel•570
    @mseidel819

    I put in a super long url, and my shortly code was cut off. Could you add a horizontal scroll for any urls that are bigger than my screen? I think that all you'll need is overflow-x: scroll or overflow-x: auto inside the css for the component that shows the long text.

    Another bonus would be to allow more than one shortened url to show up. So Instead of assigning the input to a string, you could push it into an array, then map over that array to render each url into its own container. Tools you can use:

    const urlDisplay= document.querySelector(#containeryoureaddingto); urlDisplay.insertAdjacentHTML(afterbegin, '<div>containers you'll add with each item of array</div>) const urlArray=[ ]; urlArray.map((url)=>{})`

    I made up variables, so you'll have to change them as they apply to your app. Experiment with how you'll use the map function. I think there's a couple places you could fit it.

    Marked as helpful