Skip to content

Conversation

Matt-Eva
Copy link
Contributor

No description provided.

Copy link
Contributor

@lizbur10 lizbur10 left a comment

Choose a reason for hiding this comment

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

Hey! I found a couple of issues:

  1. The solution code for Actors.js and Directors.js both use a Card component. The component is present in the solution code, but there's nothing in the instructions about creating that component. (It's also confusingly named, imo.)
  2. The tests for Actors and Directors both rely on the data.js file. I think that's fine for now - I don't think it's worth spending time on at this point - but maybe change the file name to something more explicit so it's not confusing for students.

@Matt-Eva
Copy link
Contributor Author

Good catch, thank you! I'll update that test.

In the instructions I tell students that they're free to create new components if they see fit. I made a generic card component to demonstrate component re-usability in the solution. The old solution had a nested .map statement inside a mess of JSX within a component, which I thought could have used some refactoring. But if we think this is too confusing we can change it.

@lizbur10
Copy link
Contributor

lizbur10 commented Oct 18, 2023

OK I obviously didn't read the instructions very carefully. I think that's fine. My only suggestion would be to use a more descriptive component name like ReusableCard or something. (Hopefully you can think of something better than that). Or maybe just expand the hint about creating new components to suggest they watch out for places where they have repetitive or very similar code

@Matt-Eva
Copy link
Contributor Author

Good idea! I also just pushed up two changes to update those tests. The variables are just declared directly within the files now, rather than being imported from data.js

@Matt-Eva
Copy link
Contributor Author

Ok, updated README to include more explicit hints and renamed Card component to Reusable Card!

@lizbur10 lizbur10 merged commit dd7a63f into learn-co-curriculum:master Oct 19, 2023
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