-
-
Notifications
You must be signed in to change notification settings - Fork 197
NW | 25-ITP-Sep | TzeMing Ho | Sprint 3 | Quote Generator #803
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
base: main
Are you sure you want to change the base?
Conversation
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
2 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
| updateQuoteAndAuthor(); | ||
| document | ||
| .getElementById("new-quote") | ||
| .addEventListener("click", updateQuoteAndAuthor); |
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.
You have implemented the first part of this task quite well. However, you have yet to attempt the "auto-generate" task. Why is that?
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.
I was concerned that I couldn't figure out why the second test keeps failing. So, I made this pull request to seek for guidelines. However, if it is ok to move on, I would love to try implementing new features.
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.
Hi sambabib,
I have added an auto-update feature for the quote generator.
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
3 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
@TzeMingHo for some reason the github action thinks some of the template fields have not been filled properly the only thing i can see is the title maybe? if you look at this PR #788 --> the title says Quote Generator (at the end (without dash), can you try and rename your PR? (if you get any more github action issues, try to see what the difference is against the above mentioned PR) |
|
@sambabib @mjpeet When you next look at this could you see if the tests pass? I had a look at this with @TzeMingHo in a paired session this morning and the |
is there a way for me to see these written tests? |
The test file is in the same folder named Quote-Generator in this repository. I was not supposed to make any changes to the test file, so it is not in the file changed section. |
what seems to be the issue then? |
When I tried to run the tests, the second one kept failing, even though I could actually randomly generate a new quote when I clicked the button. I was asking for a hint why this may happened. Colin had a look of the code, it may be induced by a race condition and the app didn't have enough time to update the quote to pass the test. |
|
Hello. The issue of not passing the second test has been resolved. After Jennet had very kindly looked for the different versions of userEvent used in the tests, she found that the tests were written in version 13 instead of 14, which some students had. When I reinstall "@testing-library/user-event": "^13.5.0", both tests pass. |
will you be attempting the "stretch" portion of this assignment then? |
If you are talking about the auto generator of quotes, I have already attempted to implement it. |
394c885 to
a9ba9ed
Compare
bp7968h
left a comment
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.
Nice work, some minor code improvements, otherwise looks good to me
| <label class="switch"> | ||
| <input id="toggle-btn" type="checkbox" /> | ||
| <span class="slider round"></span> | ||
| </label> |
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.
Would the screen reader be able to understand what this button is for from the context?
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.
Thank you, Bhuwan. I should have added an aria-label for the input.
Sprint-3/quote-generator/quotes.js
Outdated
| intervalId = setInterval(() => { | ||
| updateQuoteAndAuthor(); | ||
| }, 3000); |
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 the readme for this task, the submission it mentions that auto quote should have 60 seconds interval when changing. Does this auto change in 60 secs, how would you fix that. Also this 3000 in your code are known as magic numbers, is there better way to handle magic numbers
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.
Yes. I have added two variables for a preferred second and MS, which could improve readability.
bp7968h
left a comment
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.
Looks good to me!
Learners, PR Template
Self checklist
Changelist
Implemented a button to change the quote and author generated randomly from the array.
Added a toggle button for auto-displaying a new quote and author in a few seconds.
Some styling in css