Skip to content

Eval Submission#12

Open
angel-claudio wants to merge 2 commits intoprojectshft:mainfrom
angel-claudio:main
Open

Eval Submission#12
angel-claudio wants to merge 2 commits intoprojectshft:mainfrom
angel-claudio:main

Conversation

@angel-claudio
Copy link

No description provided.

setFilteredContacts(contacts)
}

console.log({ term: term })

Choose a reason for hiding this comment

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

remove console logs


return fullName.toLowerCase().includes(lowerCaseSearchTerm)
})
console.log({ before_set_contacts: foundContacts })

Choose a reason for hiding this comment

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

remove

console.log({ term: term })


const lowerCaseSearchTerm = term.toLowerCase()

Choose a reason for hiding this comment

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

you should also trim,
Will prevent using spaces in your search box.

"email": email
}

console.log(newContactData)

Choose a reason for hiding this comment

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

watch console logs

Comment on lines +21 to +23
let uniqueId = Math.round(Math.random() * 100000000).toString()

let newContactData = {

Choose a reason for hiding this comment

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

watch proper usage of const and let.


contactsApi.addContact(newContactData)

console.log(contactsApi.all())

Choose a reason for hiding this comment

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

watch console log

}

const handleClose = () => {
// router.push('/contacts')

Choose a reason for hiding this comment

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

remove commented code

{
label: "First Name",
className: "mb-3",
controlId: function () { return `form${this.label.replace(/\s/g, '')}` },

Choose a reason for hiding this comment

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

what is this doing?
this.label.replace(/\s/g, '')
Extract to function and name can help readability.


const formAttributes = {
formGroups: [
{

Choose a reason for hiding this comment

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

DRY - a lot of repeating code. You could map an array of objects and populate this form groups

generate_form_groups: function () {
let formGroups = [];

this.formGroups.forEach((formGroup) => {

Choose a reason for hiding this comment

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

using map instead of forEach achieves this.

},

addContact: function (contact) {
addContact: function (contact: Contact) {

Choose a reason for hiding this comment

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

The function itself it also needs a type

get: function (id) {
const isContact = (c) => c.id === id;
get: function (id: string) {
const isContact = (c: Contact) => c.id === id;

Choose a reason for hiding this comment

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

param has a type, but not the const

export interface Contact {
id: string;
profile_picture: string;
name?: string;

Choose a reason for hiding this comment

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

why optional?

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