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.

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

feat: adjusted grade field validation #382

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+37 −5
Open

feat: adjusted grade field validation #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62656ce
feat: adjusted grade field validation
Inferato Oct 23, 2023
8a3987e
feat: adjusted grade field validation
Inferato Oct 23, 2023
cbc4939
test: update snapshot
dyudyunov Feb 7, 2024
5aba8f2
test: increase coverage
dyudyunov Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...s/GradesView/EditModal/OverrideTable/AdjustedGradeInput/__snapshots__/index.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
exports[`AdjustedGradeInput component render snapshot 1`] = `
<span>
<Form.Control
max=""
min="0"
name="adjustedGradeValue"
onChange={[MockFunction hook.onChange]}
type="text"
type="number"
value="test-value"
/>
some-hint-text
Expand Down
14 changes: 13 additions & 1 deletion src/components/GradesView/EditModal/OverrideTable/AdjustedGradeInput/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,25 @@ const useAdjustedGradeInputData = () => {
const hintText = possibleGrade && ` ${getLocalizedSlash()} ${possibleGrade}`;

const onChange = ({ target }) => {
setModalState({ adjustedGradeValue: target.value });
let adjustedGradeValue;
switch (true) {
case target.value < 0:
adjustedGradeValue = 0;
break;
case possibleGrade && target.value > possibleGrade:
adjustedGradeValue = possibleGrade;
Copy link
Contributor

Choose a reason for hiding this comment

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

My point was that instead of artificially modifying the number the user was trying to enter, we should show an error message instead. This is so they can understand what the problem was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

The proposal makes sense, but I can't say when I'll have time to implement it (too loaded now with other stuff)

break;
default:
adjustedGradeValue = target.value;
}
setModalState({ adjustedGradeValue });
};

return {
value,
onChange,
hintText,
possibleGrade,
};
};

Expand Down
19 changes: 17 additions & 2 deletions src/components/GradesView/EditModal/OverrideTable/AdjustedGradeInput/hooks.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,26 @@ describe('useAdjustedGradeInputData hook', () => {
});
});
describe('onChange', () => {
it('sets modal state with event target value', () => {
const testValue = 'test-value';
it('sets modal state with event target value when it is less than possibleGrade', () => {
const testValue = possibleGrade - 1;
out.onChange({ target: { value: testValue } });
expect(setModalState).toHaveBeenCalledWith({ adjustedGradeValue: testValue });
});
it('sets modal state with event target value when it is equal to possibleGrade', () => {
const testValue = possibleGrade;
out.onChange({ target: { value: testValue } });
expect(setModalState).toHaveBeenCalledWith({ adjustedGradeValue: testValue });
});
it('sets modal state to possibleGrade when event target value is greater than possibleGrade', () => {
const testValue = possibleGrade + 1;
out.onChange({ target: { value: testValue } });
expect(setModalState).toHaveBeenCalledWith({ adjustedGradeValue: possibleGrade });
});
it('sets modal state to 0 when event target value is less than 0', () => {
const testValue = -1;
out.onChange({ target: { value: testValue } });
expect(setModalState).toHaveBeenCalledWith({ adjustedGradeValue: 0 });
});
});
});
});
5 changes: 4 additions & 1 deletion src/components/GradesView/EditModal/OverrideTable/AdjustedGradeInput/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@ export const AdjustedGradeInput = () => {
value,
onChange,
hintText,
possibleGrade,
} = useAdjustedGradeInputData();
return (
<span>
<Form.Control
type="text"
type="number"
Copy link
Contributor

Choose a reason for hiding this comment

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

When I tested this, I was still able to enter non-numeric characters. It could be that this is a Paragon bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's strange, I'm sure I tested that 🤔
I was able to use numbers, ., and , only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's because of the different Paragon versions. If I remember correctly - I tested it on Quince

name="adjustedGradeValue"
min="0"
max={possibleGrade || ''}
value={value}
onChange={onChange}
/>
Expand Down
Loading