Skip to content
OverviewSolutions
4
Comments
5
StarWolf
@BParadowski

All comments

  • StarWolf•260
    @BParadowskiPosted about 1 year ago

    Hey,

    after taking a look at your code it looks like you are writing Vanilla JS in React. Using DOM API like querySelector() in React is generally a last resort. Since you are bringing in a UI framework you should actually use it.

    The main problem with your implementation, from a conceptual standpoint, is that you have 2 sources of truth governing what gets rendered - React and Css classes. Let's take a look at your e-mail validation error message:

    <div className='form__text'>
  <label htmlFor="email">Email address</label>
  <span>Valid email required</span>
</div>

    You trigger the visibility of this div by changing the class. In React, this same functionality should be achieved by declaring an 'isErrorVisible" boolean variable using useState() and changing the code as follows:

     {isErrorVisible && (
        <div className="form__text">
          <label htmlFor="email">Email address</label>
          <span>Valid email required</span>
        </div>
  )}

    And of course changing the state in your submit function. You have only one class on your element this way and no reason to query any selectors. (You can also conditionally trigger classes based on the state if you really need it)

    Hope this helped

    Marked as helpful
    1
  • StarWolf•260
    @BParadowskiPosted about 1 year ago

    Hey,

    In my version of this challenge I defined the backgrounds in tailwind.config.js (because I wanted to have control of the background's position with css) and later applied them based on the page url, not props. You can take a look at the implementation here. I find it elegant enough.

    My advise with regards to components would be to have fewer of them and to create them only when they are actually needed. The same goes for props. Let's take a look at your social media links component in the footer.

    First of all, making it a component makes sense (it serves some dedicated function and can be separated from the rest of UI) but it is not necessary since you never reuse it. With regards to props however - it needs none. There is no reason to generate a (potentially different) list of social media links since there aren't that many of them. You should have simply hard-coded the links and images - no problems with props that way.

    The last piece of feedback I'd like to give you is to treat Tailwind more like a design system. You are greatly overusing arbitrary values in your code (the things inside []). By doing that you are defeating much of the purpose of using Tailwind as a framework. Recommendation: Less pixel-perfection and more of using the tools provided.

    Happy coding

    Marked as helpful
    1
  • Taufiq•150
    @taufiqmahdiSubmitted about 1 year ago
    • What is the best practice for using SVG? Should I place svg codes in a component and then export it, or there is any better way?
    • How to use Image in Next JS, what value should we pass to width and height? because it can't be passed with full or fit like tailwind.

    NFT Preview Card Component using Next JS and TypeScript

    1
    StarWolf•260
    @BParadowskiPosted about 1 year ago

    Hello again,

    • Svgs: It depends on what you want to do with an svg. If you intend to use it as you would any other image you should simply use nextjs <Image> component. If you want more granular control over it (for instance to style it with css) you can convert it to a component. There is a popular library called Svgr that does it for you (and makes the components much more readable), you can demo it in the browser here and copy-paste the component into your app.
    • Height and width attributes in next <Image> define aspect ratio of your image. I could explain further but frankly you should just read the docs. The recommended way to use images in your next js app is to import them like other assets: import image from "/assets/image...." It gets transformed into StaticImageData type and you can place this object into "src" attribute of <Image> (height and width properties are then inferred automatically)
    Marked as helpful
    1
  • Taufiq•150
    @taufiqmahdiSubmitted about 1 year ago
    • What do you think of my folder structuring? on components and interfaces.
    • What do you think of my ability to componentizing? Also is it already a good practice? Since I know I reused components for both Comment and Reply so I have to outsmart the type properties for the component.
    • What do you think of my modularization on the functions? I think I can do better but it keeps throwing error so I revert it back.
    • What do you think of my state handling and passing it to child component? is already the best practice?
    • Because this project only use Local Storage and use state in the parent page, is it okay to directly use client component? Or is there a way to make it into server component?
    • Do you think I have done good on determining the type of the state and variable used?

    Interactive Comment Section using Next and TypeScript

    1
    StarWolf•260
    @BParadowskiPosted about 1 year ago

    Okay, after taking a look at your code I have some suggestions.

    • Naming: your names are not descriptive. To be frank I have no idea what most of your code does. Component name like "ActionButton.tsx" doesn't say anything about its purpose (every button has some action/function after all). The same goes for props the like of "isReplyReply" or "onIsReplyingChange".

    • Continuing in the spirit of code readability: destructuring assignment. It allows us to do stuff like changing this:

    const onCommentVoteChange = props.onCommentVoteChange;
const commentIdToChangeVote = props.commentIdToChangeVote;
const onReplyVoteChange = props.onReplyVoteChange;
const replyIdToChangeVote = props.replyIdToChangeVote;

    into this:

    const {
  score,
  onCommentVoteChange,
  commentIdToChangeVote,
  onReplyVoteChange,
  replyIdToChangeVote,
} = props;

    • Components: you have 4 different button components which fundamentally are only visually different. A more sensible approach would be to create one component with variants.

    • Lastly, prop drilling and logic placement. There is no reason to define comment adding logic on the button! There is also no reason to make delete modal a child of the delete button... Aim to place the logic where it makes sense.

    If I were you I would focus mostly on code-readability and fighting your urge to prop-drill. I must confess that I still have no idea what is going on in most of your code.

    Marked as helpful
    1
  • jetskeeter•180
    @jetskeeter1Submitted about 1 year ago

    This is probably the most fun I have done, or rather it was relatively getting easier for me. Although, I did not finished it fast enough, but I did my best to make it the same as the image.

    Feed back is appreciated!

    Loopstudios landing page. GRID, FLEX, SCSS

    1
    StarWolf•260
    @BParadowskiPosted about 1 year ago

    Hey, off the top of my head I have 2 suggestions:

    • Your page lacks vertical alignment - each component has a distinct, different inline-margin. To fix that I suggest you use a div with a "container" class. This is how it looks on my websites (I think it is inspired by Kevin Powell you can take a look at his videos as well):
    .container {
  width: min(
    var(--container-max-width),
    100% - var(--container-margin-inline-min) * 2
  );
  margin-inline: auto;
}

    (the --container-margin-inline-min custom property changes based on screen size of course).

    • You try to achieve the "image overlap" effect by hard-coding width/margin/padding values. This is not responsive at all, for instance on my screen there is no overlap whatsoever. To achieve this effect I suggest you define a grid and place the picture and article tags inside it. Then use "grid-template" property to divide it. Finally you'd need to define "grid-area" and "z-index" properties on the picture and article elements to place them in whatever way you need.
    Marked as helpful
    1