Skip to content

London | 25-ITP-September | Ammad Ur Rehman | Sprint 3 | [TECH ED] Build quote generator app #850

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.

Sign up for GitHub

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

Jump to bottom
Closed
anosidium wants to merge 8 commits into from
Closed

London | 25-ITP-September | Ammad Ur Rehman | Sprint 3 | [TECH ED] Build quote generator app #850

anosidium wants to merge 8 commits into from

Conversation

@anosidium
Copy link

@anosidium anosidium commented Nov 15, 2025

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

Done everything including the stretch.

Questions

Nope.

@anosidium anosidium added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 15, 2025
@anosidium anosidium mentioned this pull request Nov 15, 2025
Open
bp7968h
bp7968h suggested changes Nov 24, 2025
View reviewed changes
Copy link

@bp7968h bp7968h left a comment

Choose a reason for hiding this comment

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

Nice work completing both main task and the stretch task.

I've left some questions inline about semantic HTML and variable naming, have a look into those. One thing I noticed: The stretch goal requirements mention:

"the generator should display 'auto-play:ON' somewhere on the page"

Currently, this text doesn't appear when the checkbox is checked. How would you add this feature. In relation to this feature, you immediately display new quote when checkbox is checked, this is good approach as the user will have instant feedback that the checkbox have side effect. But when you add the auto-play:ON display, do you need that? Is the side effect conveyed to the user?

@bp7968h bp7968h added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 24, 2025
@anosidium anosidium added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Nov 24, 2025
@anosidium
Copy link
Author

anosidium commented Nov 24, 2025

Thanks for the review, I have made the changes based on your suggestions.

bp7968h
bp7968h approved these changes Nov 25, 2025
View reviewed changes
Copy link

@bp7968h bp7968h left a comment

Choose a reason for hiding this comment

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

The task is well done, and meets the requirements.

@bp7968h bp7968h added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bp7968h bp7968h bp7968h approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anosidium @bp7968h @illicitonion