-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
implemented map view and example list of markers #40
base: main
Are you sure you want to change the base?
Conversation
function EventMapView({ events }) { | ||
|
||
// Example list of events | ||
events = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we indent all the code 2 spaces. I don't have the linter set up to enforce style yet, but right now the could is many indented 2 spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it is not good practice to reassign a variable passed in as a prop. Can we rename this variable for now. Maybe something like const mockEvents
for now and use them in the component below. Then I think we'll be good to merge and iterate on this component in a separate pull request. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdvchek, thanks for pushing up this pull request! Overall, this code looks good. I have two nits on the code. Also, could you give creating a placeholder test for this page?
Don't worry about trying to test the Map rendering. You can just add a placeholder title to make an assertion. See this test for an example. https://github.com/data-umbrella/event-board-web/blob/main/src/__tests__/components/pages/DonatePage.test.jsx
No description provided.