Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

solution #1988

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

solution #1988

wants to merge 2 commits into from

Conversation

Arkadiuszx
Copy link


return (
<div className="CommentList">
{comments.map(comment => (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to extract properties from comment here and pass them separately into the component, like

({ name }) => (<CommentInfo name={name} />)


export const PostList: React.FC<Props> = ({ posts }) => (
<div className="PostList">
{posts.map(post => (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link

@yevhenii-pyl yevhenii-pyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, clean and well structured job.

Could you just adjust the way you are passing props to the components? Let's use destructure them a bit.

@Arkadiuszx Arkadiuszx requested a review from yevhenii-pyl March 6, 2024 15:44
@Arkadiuszx
Copy link
Author

The demo looks now the same, but I'm having issues with the tests.

Copy link

@yevhenii-pyl yevhenii-pyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  it('should have a link with mailto: user.email', () => {
    const user1 = {
      id: 1,
      name: 'Leanne Graham',
      username: 'Bret',
      email: '[email protected]',
    };

    mount(<UserInfo user={user1} />);

    cy.get('.UserInfo').should('have.attr', 'href', 'mailto:[email protected]');
  });

Yep, noticed that thing with tests. They are expecting to receive the user object within the component.

But hey, you are a developer here, give 'em hell, fill free to change it :)
mount(<UserInfo name={user1.name} email={user1.email} />);

This is a minor adjustment to a testcase, but it makes sense due to your changes.

In this case, taking test cases into consideration, I have misguided you a bit, however, I am still convinced that it's always better to pass separate props than the object into a component whenever it's possible (and also a good practice to check and modify tests if you will).

Anyways, great job! Don't hesitate to change the world as you like it, no need to always play by the rules stated by others. 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants