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

Teacher tool: Restrict numerical inputs to no more than two digits #10168

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

kimprice
Copy link
Member

@kimprice kimprice commented Sep 10, 2024

@kimprice kimprice requested a review from a team September 10, 2024 19:52
const newValue = (e.target as any).value;
let newValue = (e.target as any).value;
if (newValue && filter) {
const pat = new RegExp(filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest moving this RegExp to a state variable so it isn't recreated on each change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved it. Does it look okay?

@thsparks
Copy link
Contributor

In firefox in the upload target, I'm still able to type in non-numeric characters (behavior seems unchanged from current version). Not sure if the changes just didn't make it into the target or if it's not working for firefox...

image

@eanders-ms
Copy link
Contributor

In firefox in the upload target, I'm still able to type in non-numeric characters (behavior seems unchanged from current version). Not sure if the changes just didn't make it into the target or if it's not working for firefox...

It's possible the target was built from out of date code. With subapps like teacher tool there's a gotcha: before creating the target, you have to rebuild pxt (gulp) to build the production version of the subapp.

@kimprice
Copy link
Member Author

It's working locally. I'll try fixing the upload target with Eric's tip.

@kimprice
Copy link
Member Author

@thsparks try it now. I updated the upload target.

const [expanded, setExpanded] = React.useState(false);
const [pattern, setPattern] = React.useState(filter ? new RegExp(filter) : undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, except I would avoid using the name pattern as the input element has an attribute by this name and we may want to expose it at some point. You could call it filter if you didn't destructure that value from props. Something like:

Suggested change
const [pattern, setPattern] = React.useState(filter ? new RegExp(filter) : undefined);
const [filter] = React.useState(opts.filter ? new RegExp(opts.filter) : undefined);

@srietkerk
Copy link
Contributor

Oh, one thing I just thought about is that this change will allow 0 as the number entered. That said, we say that some criteria aren't evaluated with a 0 entered, but not sure if we should also have that extra lower bound check.

@kimprice
Copy link
Member Author

kimprice commented Sep 11, 2024

Oh, one thing I just thought about is that this change will allow 0 as the number entered. That said, we say that some criteria aren't evaluated with a 0 entered, but not sure if we should also have that extra lower bound check.

Our current placeholder is 0, so we'd need to change that too if we wanted to disallow 0. Maybe the placeholder should be 1? Either way, if we want to do this, I think we should make the changes in a future PR.

Copy link
Contributor

@thsparks thsparks left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@eanders-ms eanders-ms left a comment

Choose a reason for hiding this comment

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

Looks good!

@kimprice kimprice merged commit 26db124 into master Sep 11, 2024
6 checks passed
@kimprice kimprice deleted the kim-inputs branch September 11, 2024 00:52
abchatra pushed a commit that referenced this pull request Sep 12, 2024
…10168)

* limit input to two digits

* move reg exp to state variable

* use 'filter' instead of 'pattern'
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.

Teacher Tools: Evaluate results is incorrect when evaluate criteria has negative number parameters
4 participants