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

Feature stopwatch #741

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

Conversation

metinbaris
Copy link
Contributor

@metinbaris metinbaris commented Jun 6, 2024

What does this change?

Adds stopwatch for the time element whenever users onClick to the minutes.

Effects common content as feature
Issue number: #642

Checklist

Who needs to know about this?

@SallyMcGrath

Copy link

netlify bot commented Jun 6, 2024

👷 Deploy request for cyf-programming pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 153b57c

Copy link

netlify bot commented Jun 6, 2024

Deploy Preview for cyf-curriculum canceled.

Name Link
🔨 Latest commit 153b57c
🔍 Latest deploy log https://app.netlify.com/sites/cyf-curriculum/deploys/6661cb2de9967e00087de427

Copy link
Member

@SallyMcGrath SallyMcGrath left a comment

Choose a reason for hiding this comment

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

Thanks so much for this. It's super fun! 🎉

I think I'd like it to make a noise when the timer runs out. Play a little tada or bring bring sound?

As it stands, this isn't accessible. Please make it keyboard focusable and assign it the role button. You will also need to communicate the beginning and end of the countdown to the announcer. This can't really work by just making it a live region, as we don't want it counting down the whole time, that would be really distracting.

Can you have a think? (And let me know if you don't know how to do these things and I would be glad to support)

common-theme/assets/styles/04-components/block.scss Outdated Show resolved Hide resolved
common-theme/layouts/partials/scripts.html Outdated Show resolved Hide resolved
@@ -11,4 +11,4 @@
{{ $blockData := .Page.Scratch.Get "blockData" }}
{{ $block := .Page.Site.GetPage $blockData.api }}
{{ $time := $blockData.time | default $block.Params.time | default 60 }}
<time class="c-block__time" datetime="P{{ $time }}M">{{ $time }} minutes</time>
<time class="c-block__time" datetime="P{{ $time }}M">{{ $time }} minutes</time>
Copy link
Member

Choose a reason for hiding this comment

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

This element cannot be accessed by keyboard. Please make it focusable and include it in the Accessibility tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SallyMcGrath, I've tried to update the code based on your feedback.

Could you please take a look at it again when you have some free time and check if it aligns with what you had in mind? If not, please feel free to edit the code directly :)

@SallyMcGrath SallyMcGrath requested review from 40thieves and Dedekind561 and removed request for 40thieves June 6, 2024 11:38
@metinbaris metinbaris marked this pull request as draft June 6, 2024 13:53
Add blink after time end, update html element for accebility
@metinbaris metinbaris marked this pull request as ready for review June 13, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔖 Ready
Development

Successfully merging this pull request may close these issues.

None yet

2 participants