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

Broken state of the Todo App - needs regex escape step for " on the id #54524

Open
code-liana opened this issue Apr 24, 2024 · 15 comments
Open
Labels
help wanted Open for all. You do not need permission to work on these. new javascript course These are for issues dealing with the new JS curriculum status: discussing Under discussion threads. Closed as stale after 60 days of inactivity. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc.

Comments

@code-liana
Copy link

Describe the Issue

When the user creates or updates the title and adds special characters ( " || ' ) the app throws an error.
Uncaught TypeError: Cannot read properties of undefined (reading 'title')

STEPS TO REPRODUCE 1:

  1. Start todo App
  2. Click on "Add New Task"
  3. On the title field add text with the quotation. Ex. "New Task"
  4. Click on "Select Task"
  5. After task created click on "Add New Task"
  6. Attempt to perform any function

ACTUAL RESULT:
Nothing happens on the screen, the application entered broken state
Uncaught TypeError: Cannot read properties of undefined (reading 'title')


STEPS TO REPRODUCE 2:

  1. Start todo App
  2. Click on "Add New Task"
  3. On the title field add text with the quotation. Ex. "New Task"
  4. Click on "Select Task"
  5. After task created click on an "Edit" button.

ACTUAL RESULT:
Nothing happens on the screen
Uncaught TypeError: Cannot read properties of undefined (reading 'title')

Affected Page

https://www.freecodecamp.org/learn/javascript-algorithms-and-data-structures-v8/learn-localstorage-by-building-a-todo-app/step-65

Your code

CODE FROM THE STEPS:

const taskObj = {
        id: `${titleInput.value.toLowerCase().split(" ").join("-")}-${Date.now()}`,
        title: titleInput.value,
        date: dateInput.value,
        description: descriptionInput.value
};

Expected behavior

EXPECTED RESULT 1:

User can add a "New Task"
For the simple solution we can add a step to add replace on id under taskObj ex. :

const taskObj = {
        //Escape special characters except space (\s) and dash (-), and leave numbers and characters 
        id: `${titleInput.value.toLowerCase().replace(/[^A-Za-z0-9\-\s]/g, '').split(" ").join("-")}-${Date.now()}`,
        title: titleInput.value,
        date: dateInput.value,
        description: descriptionInput.value
};

.replace(/[^A-Za-z0-9\-\s]/g, '') or .replace(/[^a-z0-9\-\s]/g, '')

EXPECTED RESULT 2:
User can modify the task

Screenshots

Without the fix:
without fix
With the fix:
with the fix

System

  • Device: Laptop
  • OS: Windows 11 Home
  • Browser: Chrome
  • Version: 124.0.6367.61

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/124.0.0.0 Safari/537.36

Additional context

There is other enhancement that can be made on the app, by trimming the title string to make sure it's not empty or blank spaced. Otherwise user can create an empty title even though the field is required on HTML.

This is what I did, but I'm not good in coding yet, so I don't feel confident to report it and give suggestions on the solution.

JS

taskForm.addEventListener('submit', (e) => {
    e.preventDefault();
    if (!isEmpty(titleInput.value)) {
        addOrUpdateTask();
    } else {
        // Create an error message element
        errorMessage.textContent = "Error: title cannot be an empty value!";
        errorMessage.style.display = "block";

        // Remove the error message after 3 seconds
        setTimeout(() => {
            errorMessage.style.display = "none";
        }, 3000);
    }
});

HTML

<div class="task-form-body">
          <label class="task-form-label" for="title-input">Title</label>
          <input required type="text" class="form-control" id="title-input" value="" />
          <label id="errorMessage" style="display: none; color: red; font-weight: bold;"></label>
          <label class="task-form-label" for="date-input">Date</label>
          <input type="date" class="form-control" id="date-input" value="" />
          <label class="task-form-label" for="description-input">Description</label>
          <textarea class="form-control" id="description-input" cols="30" rows="5"></textarea>
        </div>
@code-liana code-liana added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. status: waiting triage This issue needs help from moderators and users to reproduce and confirm its validity and fix. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc. labels Apr 24, 2024
@gikf gikf added the new javascript course These are for issues dealing with the new JS curriculum label Apr 25, 2024
@gikf
Copy link
Member

gikf commented Apr 25, 2024

This is nice finding. I wasn't able though to confirm single quote ' is causing the same issue. Could you check again?

A little more context, after item with double quote is added - ie. "f", this is how looks its div .

<div class="task" id="" f"-1714051100417"="">
          <p><strong>Title:</strong> "f"</p>
          <p><strong>Date:</strong> </p>
          <p><strong>Description:</strong> </p>
          <button onclick="editTask(this)" type="button" class="btn">Edit</button>
          <button onclick="deleteTask(this)" type="button" class="btn">Delete</button> 
        </div>

With the messed up id deleting item also doesn't work correctly. It might appear otherwise. The correct item is removed from the displayed list, however from the internal taskData and localStorate removed is last item, as it's spliced: taskData.splice(dataArrIndex, 1), with dataArrIndex == -1.

@gikf gikf changed the title Broken state of the Todo App - needs regex escape step for ( " || ' ) on the id - Learn localStorage by Building a Todo App Broken state of the Todo App - needs regex escape step for ( " || ' ) on the id Apr 25, 2024
@jdwilkin4
Copy link
Contributor

Hi @code-liana !

Thanks for reporting this

I think it would be better to create a separate function for removing special characters and trim the ends for whitespace
That way it can be used for the id, title and description

const removeSpecialChars = (val) => {
  return val.trim().replace(/[^A-Za-z0-9\-\s]/g, '')
}

const addOrUpdateTask = () => {
  addOrUpdateTaskBtn.innerText = "Add Task";
  const dataArrIndex = taskData.findIndex((item) => item.id === currentTask.id);
  const taskObj = {
    id: `${removeSpecialChars(titleInput.value).toLowerCase().split(" ").join("-")}-${Date.now()}`,
    title:removeSpecialChars(titleInput.value) ,
    date: dateInput.value,
    description: removeSpecialChars(descriptionInput.value),
  };

As for checking for empty titles, I personally like showing an alert instead of creating a new error message and styling it

const isTitleEmpty = (val) => !val.trim().length


taskForm.addEventListener("submit", (e) => {
  e.preventDefault();

  if(isTitleEmpty(titleInput.value)){
    alert("Please provide a title")
    return
  }
  
  addOrUpdateTask();
});

let's hear what the others think.
I'll open this up for conversation

Also, sidenote @naomi-lgbt and @Ksound22 the creation of these smaller functions would work well for what we were talking about early about updating how the breakdown of projects is done.

@jdwilkin4 jdwilkin4 added the status: discussing Under discussion threads. Closed as stale after 60 days of inactivity. label Apr 25, 2024
@code-liana
Copy link
Author

code-liana commented Apr 25, 2024

@gikf
Thanks :) If you are looking for volunteer QA, I would be happy to join ;)

Actually you are right, it doesn't happen with the single quotes in case if you have double quotes on you taskContainer.innerHTML. I overlooked that before I had single quotes on id under taskContainer.innerHTML so that was my bad, for not double checking.

const updateTaskContainer = () => {
    tasksContainer.innerHTML = ""
    taskData.forEach(
        ({ id, title, date, description }) => {
            (tasksContainer.innerHTML += `
          <div class="task" id='${id}'>
            <p onClick="isEmpty(this)"><strong>Title:</strong> ${title}</p>
            <p><strong>Date:</strong> ${date}</p>
            <p><strong>Description:</strong> ${description}</p>
            <button onclick="editTask(this)" type="button" class="btn">Edit</button>
            <button onclick="deleteTask(this)" type="button" class="btn">Delete</button>
          </div>
        `)
        }
    );
};

WHERE:

<div class="task" id='${id}'> 

In this case the error will be the same as in previous case.

As a result I still would suggest to write a solution for both scenarios. HTML allows us to use double or single quotes.

@naomi-lgbt naomi-lgbt removed the status: waiting triage This issue needs help from moderators and users to reproduce and confirm its validity and fix. label Apr 25, 2024
@code-liana
Copy link
Author

@jdwilkin4
Separate function is definitely an option, but why do we need it. The issue is only due to an id field. User still can have the flexibility on what is displayed on the title and description field. Otherwise I feel like it would be better to let user know before he creates the task that Title and Description field cannot accept special characters.

For the white space and blank title I think it would be better to move it even into a separate enhancement(issue) as it seems to be completely unrelated

@jdwilkin4
Copy link
Contributor

@code-liana

You would still need to remove the quotes for the title though otherwise the error shows up

Screenshot 2024-04-25 at 12 06 11 PM

which results in this id

Screenshot 2024-04-25 at 12 14 50 PM

You could have something like this

 const taskObj = {
    id: `${titleInput.value.replace(/[`'"]/g,'').toLowerCase().split(" ").join("-")}-${Date.now()}`,
    title: titleInput.value.replace(/[`'"]/g,''),

which would fix at least the quotes issue.

but then, IMO, you would be missing out on have a good teaching opportunity and just have campers create a separate function for that and use that.

@code-liana
Copy link
Author

@jdwilkin4
This error shows up due to an id.
<div class="task" id="${id}"> is an attribute, and it messes up when we add double quotes after the equality. when we send it it looks like id=""new-task"-34823094" first two double quotes already is like an empty attribute value for the attribute id and all rest is just some weird text that system probably cant read.

Title on the other hand is just a text value(content) that automatically handling the escape characters on the value string.
<p onClick="isEmpty(this)"><strong>Title:</strong> ${title}</p>


I tried to reproduce the issue with the error you displayed, but not adding code change for the title field title: titleInput.value.replace(/[`'"]/g,''), and I'm unable to reproduce this issue
Screenshot 2024-04-25 152615


Side note: If you created broken task and then fixed the code, the task that is already created will be broken.

@jdwilkin4
Copy link
Contributor

jdwilkin4 commented Apr 25, 2024

This error shows up due to an id.
<div class="task" id="${id}"> is an attribute, and it messes up when we add double quotes after the equality.

Yeah, I get that.

When I initially tested it earlier, I forgot to apply the changes for the id so the error message was showing up for the title too.

It looks like then we could just apply the replace and IMO the trim too so you don't get ids like this

Screenshot 2024-04-25 at 1 25 07 PM

so it looks like maybe this would an additional 2 steps

@jdwilkin4
Copy link
Contributor

For the white space and blank title I think it would be better to move it even into a separate enhancement(issue) as it seems to be completely unrelated

if you want to create a separate issue for that then that is fine

@jdwilkin4
Copy link
Contributor

@code-liana

This is what I did, but I'm not good in coding yet, so I don't feel confident to report it and give suggestions on the solution.

When reporting issues, you don't always need to have a solution in the issue.
You can just report the issue and the mods will open it up for discussion and the community can discuss different solutions.
Then the mods can choose a path forward from there

@Ksound22
Copy link
Member

This is nice finding. I wasn't able though to confirm single quote ' is causing the same issue. Could you check again?

A little more context, after item with double quote is added - ie. "f", this is how looks its div .

<div class="task" id="" f"-1714051100417"="">
          <p><strong>Title:</strong> "f"</p>
          <p><strong>Date:</strong> </p>
          <p><strong>Description:</strong> </p>
          <button onclick="editTask(this)" type="button" class="btn">Edit</button>
          <button onclick="deleteTask(this)" type="button" class="btn">Delete</button> 
        </div>

With the messed up id deleting item also doesn't work correctly. It might appear otherwise. The correct item is removed from the displayed list, however from the internal taskData and localStorate removed is last item, as it's spliced: taskData.splice(dataArrIndex, 1), with dataArrIndex == -1.

Single quote isn't causing it on my end too

@Ksound22
Copy link
Member

This is a great finding! We should open it up for contribution right away.

@Ksound22
Copy link
Member

Hi @code-liana !

Thanks for reporting this

I think it would be better to create a separate function for removing special characters and trim the ends for whitespace That way it can be used for the id, title and description

const removeSpecialChars = (val) => {
  return val.trim().replace(/[^A-Za-z0-9\-\s]/g, '')
}

const addOrUpdateTask = () => {
  addOrUpdateTaskBtn.innerText = "Add Task";
  const dataArrIndex = taskData.findIndex((item) => item.id === currentTask.id);
  const taskObj = {
    id: `${removeSpecialChars(titleInput.value).toLowerCase().split(" ").join("-")}-${Date.now()}`,
    title:removeSpecialChars(titleInput.value) ,
    date: dateInput.value,
    description: removeSpecialChars(descriptionInput.value),
  };

As for checking for empty titles, I personally like showing an alert instead of creating a new error message and styling it

const isTitleEmpty = (val) => !val.trim().length


taskForm.addEventListener("submit", (e) => {
  e.preventDefault();

  if(isTitleEmpty(titleInput.value)){
    alert("Please provide a title")
    return
  }
  
  addOrUpdateTask();
});

let's hear what the others think. I'll open this up for conversation

Also, sidenote @naomi-lgbt and @Ksound22 the creation of these smaller functions would work well for what we were talking about early about updating how the breakdown of projects is done.

Do we need a separate function to handle removing special characters at all?

@jdwilkin4
Copy link
Contributor

@Ksound22

Do we need a separate function to handle removing special characters at all?

No

You can see my reply here about the fix we are going with
#54524 (comment)

@jdwilkin4 jdwilkin4 added help wanted Open for all. You do not need permission to work on these. and removed scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. labels Apr 27, 2024
@code-liana
Copy link
Author

code-liana commented Apr 30, 2024

if you want to create a separate issue for that then that is fine

@jdwilkin4 Created a separate issue for the white space
#54588

And actually found another one that related to "Add Task" and "Update Task".
#54589

@code-liana
Copy link
Author

Single quote isn't causing it on my end too

Hi @Ksound22 I think I already mentioned it somewhere above. It's reproduced if your HTML code in JS has single quotes and not double
#54524 (comment)

@gikf gikf changed the title Broken state of the Todo App - needs regex escape step for ( " || ' ) on the id Broken state of the Todo App - needs regex escape step for " on the id May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Open for all. You do not need permission to work on these. new javascript course These are for issues dealing with the new JS curriculum status: discussing Under discussion threads. Closed as stale after 60 days of inactivity. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc.
Projects
None yet
Development

No branches or pull requests

5 participants