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

Johannas Chatbot #277

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

Johannas Chatbot #277

wants to merge 21 commits into from

Conversation

joheri1
Copy link

@joheri1 joheri1 commented Sep 6, 2024

Netlify link

Add your Netlify link here.
PS. Don't forget to add it in your readme as well.

@joheri1 joheri1 marked this pull request as draft September 6, 2024 11:44
@joheri1 joheri1 marked this pull request as ready for review September 6, 2024 11:44
Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Good job on creating this project, and handing in so early (?!) 💪 There are some things I'd like you to update, but let's go through all the feedback first.

HTML/CSS

  • You kept most of the starter code as it was, but updated IDs to fit your project better. Nice! Also, nice touch with the start button. Just be mindful when using abbreviation, maybe start-button is OK instead of start-btn?
  • Have a look at how your chat looks on small mobiles. You have some long words in the chatbot responses that make it look cramped (text is overflowing the message box).
  • The footer should be moved within the body tag

JavaScript

  • Good use of let for global variables (userName, coffeeChoice, etc.), as their values change during the chat flow.
  • Functions are broken down into manageable pieces, each handling a specific part of the chat flow (e.g., handleUserInput, selectCoffee). This modularity is a great practice!
  • Consider organizing functions in a more logical sequence, e.g., placing utility functions (showMessage, etc.) before those that rely on them (greetUser, handleUserInput). See more about order in the link I posted as a standalone comment.

Clean Code

  • Remember to be consistent with whether you use single or double quotes
  • Remember to remove all console.logs before handing in. They are great for developing but are not needed in production mode
  • The CSS has some repeated styles for buttons (e.g., .size-btn and .btns). These could be refactored into a shared class to avoid repetition.
  • Variable and function names are generally descriptive, making the code easy to understand (showMessage, handleUserInput, etc.). Maybe userAnswer can follow this same practice, and be called handleSelectedCoffeShop or something?
  • Be consistent with whether or not you're using semicolons. We don't want to see a mix 👀
  • It's OK to have different type of input fields for different questions, but for questions where you expect one option out of a set of specific answers, such as the type of coffee question, it would be better to use buttons, select or a group of radios. Right now it appears buggy if I write something that doesn't exist.
    Skärmavbild 2024-09-18 kl  10 28 03
  • Plus, something's wrong with the order of the answers there ☝️
  • Remove the name input after it's used and replace it with the new user interactions, such as the button. Right now the buttons appear within the chat window, but you can't see them unless you scroll down.

Requested Changes

  • Clean up the code: remove console.logs, be consistent with whether or not you're using single/double quotes and semicolons
  • Clean up the user flow: don't show the user name input before the user should write their name + remove the user name input after the user has written their name. If we're not doing this, we're allowing the user to mess with the flow and the chatbot appears buggy. Keep the user interaction elements outside of the chat to avoid confusion

Overall, you've done an impressive job and I like that you made this project your own and didn't "copy" the Technigo example. However, with these requested changes, it's gonna be even more impressive and user-friendly.

PS. Please, reach out on Stack Overflow if you have any questions or need any guidance in updating your code. I know feedback like this can feel overwhelming, so remember we're here for you if you have any questions or need any clarifications 🫶


## If you had more time, what would be next?

**1.** Figure out a way to move line 17-27 further down in the script. It looks awkward to me to have "Handle user name input" above the "Start conversation", but the code broke when I moved it so I didn't dare to continue since time was running out.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good approach! Have a look at the notes from last week's Thursday session to learn more

Comment on lines +19 to +21
**3.** I used `nameForm.removeEventListener("submit", handleUserInput)` because I wanted the "Start conversation" button to be the only visible option in the chat, forcing the user to click it to begin, but the event listener for the name form is still active. If I had more time, I would address this issue.

**4.** The buttons seem to have a life of their own, and the input field remains visible in the background (likely related to the previous issue). If I had more time, I would fix this, but at this point, I'm just glad they're working. I struggled with this earlier in the project and almost abandoned the idea altogether.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for sharing your progress Johanna! In this project it's important to guide the user on what they need to be doing, so I would like you to actually look into this. It shouldn't be possible for the user to write their name:

  • before clicking the Start button
  • after submitting their name once already

…tion message. Change the buttons to have the same layout. Hide the name input field when the Start conversation button is visible. Remove the start conversation button when the name input field is visible. Remove name input field after submit. Add CSS to style the selectbuttons and the name input field. Add requestAnimationFrame to avoid scrolling to see the buttons on the form
…ablet. Add setTimeout to create a better flow in the chat conversation.
@joheri1
Copy link
Author

joheri1 commented Sep 19, 2024 via email

Copy link

@JoyceKuode JoyceKuode left a comment

Choose a reason for hiding this comment

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

Thank you for leaving a lot of comments throughout your JavaScript file. This helped make it easy to navigate through the steps and follow the flow ☺️ Overall, I think your chatbot works as intended, your code is very well-organized, and I was happy to be able to order a pumpkin spice latte in Gothenburg ☕ 🎃 😋


## The problem
**1. Creativity went wild**
When I started working on my project, my creativity took over when I read, "Step 2 - Have it your way 👋". I envisioned a coffee shop chatbot, like a Starbucks order flow, but soon realized that my idea was far too complex. Options for hot or cold drinks, different coffee shop locations, milk choices depending on the coffee type, drop-downs, buttons - the scope was way beyond my skills. Instead of realizing this right away, I stumbled across more issues as I progressed, forcing me to simplify my plan. I read through all the instructions before picking up my code again, and limited the scope to be more like my previous project (Project Javascript Pizzeria) with a very limited amount of options for the user, similar if else statement, etc.

Choose a reason for hiding this comment

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

Thank you for sharing your approach, I found this very relatable - especially the creativity going wild and attention to detail issues 😅

const handleCoffeeShopInput = () => {
inputWrapper.classList.add("hidden")
showMessage("Where would you like to pick up your coffee?", 'bot')
chat.innerHTML += `

Choose a reason for hiding this comment

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

I noticed that you use append += with innerHTML frequently throughout your code. (I just used innerHTML = (without the +)) I also noticed that you frequently used .remove() .. I wonder if these are related. If you took out the append with innerHTML would you need to use remove() ? Because I think innerHTML = automatically replaces whatever was there previously, and therefore you could probably simplify a lot of the code.

Copy link
Contributor

@JennieDalgren JennieDalgren left a comment

Choose a reason for hiding this comment

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

⭐ ☕

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.

4 participants