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

Adding Testing with Vite #1908

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

Adding Testing with Vite #1908

wants to merge 2 commits into from

Conversation

Yosolita1978
Copy link
Collaborator

No description provided.

@@ -1,16 +1,23 @@
import './App.css'
import 'bootstrap/dist/css/bootstrap.min.css';
import MyNavBar from './components/Navbar'
import MyNavBar from './routes/Navbar'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally would leave NavBar in the components folder because that's what NavBar is, a shared component used on multiple pages; a route would be a page/view*.

* At least in the traditional/beginner setup

<MyNavBar />
<ListStudents />

<MyNavBar handleMe={handleMe} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

React convention is that prop names that are callbacks start with the word "on" and then a description of what event it is. So since this callback is called whenever the account menu item is clicked, I'd rename this prop to onAccountMenuItemClicked or something along those lines.

And then handleMe can be renamed to handleAccountMenuItemClicked since "handle" is the conventional suffix used for implementations of callback props

<div id="contact">
<div>
<img
key={contact.avatar}
Copy link
Collaborator

Choose a reason for hiding this comment

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

A key prop is not necessary unless you're in a loop OR are abusing the key prop to explicitly trigger re-renders (which doesn't look to be the case here)

</>
) : (
<i>No Name</i>
)}{" "}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
)}{" "}
)}

Comment on lines +46 to +49
</div>
</div>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
</div>
</div>
)
}
</div>
</div>
)
}

Comment on lines +21 to +40
/*
describe('event signup form', () => {
it('renders a form', () => {
render(<EventSignUpForm />);

const signUpForm = screen.getByTestId('event-form');
expect(signUpForm).toBeInTheDocument();
});
});


test('test that MyNavBar renders', () => {
// const navbarElement = render(<MyNavBar/>);
// const navbarId = navbarElement.getByTestId('navbar');
// expect(navbarId).toBeDefined();
// });



*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/*
describe('event signup form', () => {
it('renders a form', () => {
render(<EventSignUpForm />);
const signUpForm = screen.getByTestId('event-form');
expect(signUpForm).toBeInTheDocument();
});
});
test('test that MyNavBar renders', () => {
// const navbarElement = render(<MyNavBar/>);
// const navbarId = navbarElement.getByTestId('navbar');
// expect(navbarId).toBeDefined();
// });
*/

Looks like unused code?

Comment on lines +10 to +19
// /*
// const router = createBrowserRouter(
// createRoutesFromElements(
// <Route path="/" element={<App />}>
// <Route path="login" element={<Contact />} />
// </Route>
// )
// );

// */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove unused code to prevent confusion OR write an explanation on what this commented out code does

Open-Source Curriculum TO-DO Board automation moved this from To do to Needs Reviewed Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants